tags:

views:

97

answers:

3

EDIT:

In the following code container::push takes an object of type T that derives from base as argument and stores in a vector a pointer to the method bool T::test().

container::call calls each of the stored methods in the context of to the member object p, which has type base, not T. It works as long as the called method does not refer to any member outside base and if test() is not declared virtual.

I know this is ugly and may not be even correct.

How can I accomplish the same thing in a better way?

#include <iostream>
#include <tr1/functional>
#include <vector>

class base {
   public:
   base(int v) : x(v)
   {}
   bool test() const { // this is NOT called
      return false;
   }

   protected:
   int x;
};

class derived : public base {
   public:
   bool test() const { // this is called instead
      return (x == 42);
   }
};

class container {
   public:
   container() : p(42)
   {}
   template<typename T>
   void push(const T&) {
      vec.push_back((bool (base::*)() const) &T::test);
   }
   void call() {
      std::vector<bool (base::*)() const>::iterator i;
      for(i = vec.begin(); i != vec.end(); ++i) {
         if( (p .* (*i))() ) {
            std::cout << "ok\n";
         }
      }
   }

   private:
   std::vector<bool (base::*)() const> vec;
   base p;
};

int main(int argc, char* argv[]) {
   container c;
   c.push(derived());
   c.call();
   return 0;
}
+3  A: 

What you are doing with your "boost::bind" statement is to call derived::test and pass "b" as a "this" pointer. It's important to remmember that the "this" pointer for derived::test is supposed to be a pointer to a "derived" object - which is not the case for you. It works in your particular situation since you have no vtable and the memory layout is identical - but as soon as that will change, your program will likely break.

And besides, it's just plain wrong - ugly, unreadable, bug-prone code. What are you really trying to do?

[Edit] New answer to the edited question: You should use boost::bind to create a functional closure, that wraps both the object & the member function in a single object - and store that object in your collection. Then when you invoke it, it is always reliable. If you can't use boost in your application... well, you could do something like boost::bind yourself (just look on how it is done in boost), but it's more likely that you'll get it wrong and have bugs.

Virgil
I rewrote the question, is it clear now?
Helltone
+1  A: 

What you are doing is not correct, and in the simple example it will work, but might just raise hell (one of the possibilities for undefined behavior) in other cases.

Since base::test and derived::test are not virtual, they are two different member methods, so for simplicitly I will call them base::foo and derived::bar. In the binder code you are forcing the compiler into adapting a pointer to bar that is defined in derived as if it was actually defined in base and then calling it. That is, you are calling a method of derived on an object or type base!!! which is undefined behavior.

The reason that it is not dying is that the this pointers in base and derived coincide and that you are only accessing data present in the base class. But it is incorrect.

When you declare base::test virtual, you get the correct behavior: your most derived object in the hierarchy is base, the compiler will use the virtual dispatch mechanism and find out that base is where the final overrider for test is found and executed.

When you declare only derived::test as virtual (and not base) the compiler will try to use an inexistent virtual dispatch mechanism (usually a vtable pointer) in the handed object and that kills the application.

At any rate, all but the virtual base::test uses are incorrect. Depending on what your actual requirements are, the most probably correct way of doing it would be:

class base {
public:
   virtual bool test() const;
};
class derived : public base {
public:
   virtual bool test() const; // <--- virtual is optional here, but informative
};
int main()
{
   derived d; // <--- the actual final type
   base & b = d;  // <--- optional
   if ( std::tr1::bind( &base::test, std::tr1::ref(b))() ) {
      // ...
   }
}

Note that there is no cast (casts are usually a hint into something weird, potentially dangerous is hiding there), that the object is of the concrete type where you want the method to be called, and that the virtual dispatch mechanism guarantees that even if the bind is to base::test, as the method is virtual, the final overrider will be executed.

This other example will more likely do funny things (I have not tried it):

struct base {
   void foo() {}
};
struct derived : base {
   void foo() { 
      for ( int i = 0; i < 1000; ++i ) {
         std::cout << data[i];
      }
   }
   int data[1000];
};
int main() {
   base b;
   std::tr1::bind((void (base::*)()) &derived::foo, std::tr1::ref(b))();
}
David Rodríguez - dribeas
Thanks for the explanation but shouldn't std::tr1::ref(b) be std::tr1::ref(d) in your example?
Helltone
Uhm... the actual problem is that I forgot to initialize the referece. The code would not even compile. Anyway with the corrected code, you can use either `b` or `d`, as they both refer to the same object and both are `base`'s
David Rodríguez - dribeas
+1  A: 

To the updated question:

Calling a derived member function on a base object is Undefined Behavior. What you are trying to achieve (code) is wrong. Try to post what you need and people will help with a sensible design.

David Rodríguez - dribeas