Back to General and Gameplay Programming

invoking Base::virtualMethod instead of Derived

General and Gameplay Programming Programming

Started by nlbs November 08, 2008 09:51 AM

23 comments, last by nlbs 15 years, 5 months ago

nlbs

100

Author

November 08, 2008 09:51 AM

In my class Js::Object I've an operator() overload which returns Js::JType* all Other Classes in Js Namespace e.g. Js::Number, Js::String etc.. even Js::Object inherits from Js::JType

Js::JType* &operator()(const string& key);

source


/**
typedef std::map<string, Js::JType*> ElemMap;
*/
Js::JType* &Js::Object::operator()(const string& key){
	ElemMap::iterator it=list.find(key);
	if(it != list.end()){
		return (Js::JType* &)(it->second);
	}else{
		Js::JType* x = new Js::JType;
		list.insert(make_pair(key, x));
		return x;
	}
}

and this is how I am using it.

Js::Object o;
o("math") = new Js::String("Hello World");

Now If everything is ok.

o("math")->commit()

should return "Hello World" (with quotes as std::string) then it will invoke Js::String::commit() But Its invoking Js::JType::commit() and thus its returning null (as std::string) e.g. in other words its invoking the commit() of grand parent Class not of the intended class. and yes commit is virtual. Hope I am able to clarify the problem.

Oxyd

1,162

November 08, 2008 10:06 AM

The problem is, when you get an iterator into a std::map, the values held are copied. So your op() returns a reference to a local copy of the pointer, not a reference to the pointer stored in the map itself.

I don't think there's any sensible way to make it work the way you intended. But you could have your op() return just the pointer that would then be used for access to the pointed-to object, and provide another function for inserting values into the map.

hh10k

589

November 08, 2008 10:08 AM

As Oxyd said, "return x" is returning the address of a local variable, and as soon as x leaves scope that address no longer exists. You should be returning list.insert(make_pair(key, x)).first.second, which is the address of the newly inserted value.

rip-off

11,000

November 08, 2008 10:09 AM

You have a memory leak. You are returning a reference to a pointer allocated with "new", and then overwriting it with another pointer allocated with "new", without a "delete" call.

It is semantically incorrect. The "else" part does not return a reference to a pointer in the map, it returns a reference to a local variable. I think this is the cause of your error.

I believe your code could be expressed shorter:

Js::JType* &Js::Object::operator()(const string& key){	ElemMap::iterator it=list.find(key);	if(it != list.end()){		// (Js::JType* &) is unnecessary		return it->second;	}else{                JS::JType *ptr = 0;		list.insert(make_pair(key, ptr));		return list[key];	}        }

This code doesn't leak - but it could potentially return NULL when the key isn't found. The calling code would have to be able to handle this.

An alternative would be to use smart pointers.

In fact, I believe the code code potentially be shortened to:

Js::JType* &Js::Object::operator()(const string& key){	return list[key];        }

The only issue is if a default constructed pointer (as opposed to an uninitialised pointer) is *guarenteed* to be 0. I think it might be, but I never store raw pointers in containers so I've never looked into it.

blog | twitter

nlbs

100

Author

November 08, 2008 10:25 AM

Thanks it worked.

Antheus

2,410

November 08, 2008 11:42 AM

I have questioned the sanity of such approach before. But if well defined, and carefully coded, it can work.

Personally, I once used such approach, but I used templates to specify return types, resulting in compile-time checks to avoid many problems.

To use fully run-time evaluated system, this is how I would approach it (using a proxy holder for return types).

#include <iostream>#include <string>#include <map>struct JType {	JType()          { std::cout << "JType()" << std::endl; }	virtual ~JType() { std::cout << "~JType()" << std::endl; }	virtual void commit() = 0;};struct JDefault : public JType {	JDefault()    { std::cout << "JDefault()" << std::endl; }	~JDefault()   { std::cout << "~JDefault()" << std::endl; }	void commit() {	std::cout << "Commit not supported" << std::endl; }};struct JString : public JType {	JString()     { std::cout << "JString()" << std::endl; }	~JString()    { std::cout << "~JString()" << std::endl; }	void commit() {	std::cout << "Hello World" << std::endl; }};class Object {	typedef JType * T;	typedef std::map<std::string, T> List;	// non-copyable, too messy to implement	Object(const Object &) {}	Object operator=(const Object &) {}	struct Proxy {		friend class Object;		Proxy & operator=(T rhs) {			clear();			ref = rhs;			return *this;		}		bool exists() const { return ref != NULL; }		void clear() const { delete ref; ref = NULL; }		T operator->() const { return value(); }		T operator*() const { return value(); }	private:		Proxy(const List::iterator & i) : ref(i->second) {}		Proxy(const Proxy & other) : ref(other.ref) {}		T value() const { 			static JDefault def;			return (ref == NULL) ? &def : ref; 		}		// assignment can't be implemented		Proxy & operator=(const Proxy &) { }		T & ref;	};	List list;public:	Object() {}	~Object() {		List::iterator i = list.begin();		for (List::iterator i = list.begin(); i!= list.end(); ++i) delete i->second;			}	Proxy operator[](const std::string & key) {		List::iterator i = list.find(key);		if (i != list.end()) {			return Proxy(i);		} else {			return Proxy(list.insert(list.end(), std::make_pair(key, T(NULL))));		}	}	bool has(const std::string & key) {		return list.count(key) != 0;	}};int main(int argc, char* argv[]){	Object o;	o["math"] = new JString();	if (o["empty"].exists()) std::cout << "'empty' should not exist" << std::endl;	o["math"]->commit();	o["math"].clear();	o["math"]->commit();	o["empty"]->commit();}

I have tried to think of all the corner cases while limiting the code to nothing and absolutely nothing beyond the given problem. Even with that I'm not sure I got it right (C++ is annoying in that way).

In above case Proxy is temporary holder for reference to actual pointer which resides inside std::map's node. Proxy should be transient object that cannot persist beyond scope of caller. It also relies on map not being changed while its alive.

In release builds, Proxy is completely eliminated, all operations on it result in query of actual holder of pointer.

Lastly, there's the performance penalty to consider. Each time a variable is accessed, map must be searched. This could be solved by allowing Proxy to be cached, but that throws safety out of window.

Finally: std::map is designed the way it is for a good reason. Unlike managed objects, where NULL/null is a given, map[] may result in undefined behavior. It is almost without exception better to use the map properly rather than trying to go against the design of C++.

Also - Python, PHP, Lua or similar would be much better choice to implement system like this. C++ is simply the wrong language for this.

nlbs

100

Author

November 08, 2008 12:10 PM

Ya this way I think would be good.
The mechanism you have used is stl containers way.
I thought this mechanism and named it JAdapter (cause I want both JArray and JObject would share it) anyway as you brought the Friendly Proxy mechanism lets talk about that.
But 1 question.

JNumber x = "Hello World";Object o;o["x"] = x;o["x"] = new JString("y");//Here x will be used in some other purpouse too.std::cout<<x.toStdString()<<std::endl;

Now upon reassignment clear() is being called can x be called after o["x"] has been reassigned as you are doing delete ref on clear()??

If you delete that then it cant be used in other purpouse latter after reassignment.

and if you don't delete it would cause memory leak.

So what can be done in this situation !!

Antheus

2,410

November 08, 2008 12:29 PM

Quote:Original post by nlbs
Now upon reassignment clear() is being called can x be called after o["x"] has been reassigned as you are doing delete ref on clear()??

If you delete that then it cant be used in other purpouse latter after reassignment.

and if you don't delete it would cause memory leak.

So what can be done in this situation !!

Nothing, since it wouldn't even compile.

What you meant was:

	JString js;	o["a"] = &js

I don't care about this case, since you deliberately ignore it in your design. If you require user to allocate the objects, and container must release them, that's generally fragile design.

I've said it before, the best way is to not do things this way, or look at competing approaches, such as boost::any.

But most importantly, C++ is really poor language to implement such features. It can be done, but full implementation involves a lot of magic template fu and various dynamic tests. These techniques have been explored several times, but they remain mostly obscure oddities with little practical use.

In current implementation objects managed by container must derive from JType and be allocated with new. Working around that would require JType to properly implement cloning and more. There really is a good reason why map works the way it works.

If anything, you might want to try:

template < class U >Proxy & operator=(const U & rhs) {	clear();	ref = new U(rhs);	return *this;}

That allows you to manage polymorphic behavior inside the container, and let compiler take care of type safety. This would be safe to use with auto-allocated objects as well, while retaining polymorphic behavior. It does cause problems when assigning base class. You could provide a virtual clone() method or something.

	Object o;	JString js;	o["a"] = js;	o["math"] = JString();

nlbs

100

Author

November 08, 2008 11:32 PM

In your Proxy class why are you doing T & ref;
why not just T . cause JType* would store a pointer but I cant understand why a reference to a pointer is required.

and secondly I cant understand why Object Class is non-copyable

Antheus

2,410

November 09, 2008 07:47 AM

Quote:Original post by nlbs
In your Proxy class why are you doing T & ref;
why not just T . cause JType* would store a pointer but I cant understand why a reference to a pointer is required.

Because you need to modify the pointer, not the contents of pointer. Simple example.

typedef JType * JTypePtr;void assign(JTypePtr & ptr, const JTypePtr & newValue) {  delete ptr;  ptr = newValue;};...std::pair<std::string, JType *> entry.assign(entry, new JString());

It would be possible to use C style syntax, and store JType ** instead. But given the nature of the problem, that is not necessary.

Quote:and secondly I cant understand why Object Class is non-copyable

Because map contains raw pointers that it needs to clean up. Whenever you copy Object, you need to copy the contents of map. This is impossible due to polymorphic nature of JType *. To make map copyable, you first need to provide JType with virtual constructor, then implement copy constructor and assignment operator correctly for Object.

invoking Base::virtualMethod instead of Derived

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

invoking Base::virtualMethod instead of Derived

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines