Archived

This topic is now archived and is closed to further replies.

NextS

intersting behaviour of std::map

Recommended Posts

Hi all. Yes you may say I should have known this, but anyway here''s an interesting way std::map behaves: (Code is not recommended ) I once again spend some time figuring out where the bug was (I built a messaging system into my app which has become somewhat big recently so debugging was a little bit difficult. And I did absolutely not see why this code could have been wrong (this is a simplified version).
  
#include <iostream>
#include <map>
#include <string>

using namespace std;

class A;

class A
{
	public:
		A ( void):mThis(this) {}
		virtual void notify ( void )
		{
			mThis->callMe ();
		}
		virtual void callMe ( void )
		{
			cerr << "A"<<endl;
		}
	private:
		A *mThis;
};

class B: public A
{
	public:
		void callMe (void)
		{
			cerr << "B"<<endl;
		}
};

void main (int argc, char **argv )
{
	map<string,B> myMap;
	myMap["A"].notify();
	myMap["A"].notify();
}

  
I tested the code with g++ and Codewarrior and both Compilers behaved the same way (crashing the app at the second call to notify). To draw a conclusion: Never forget your nice and friendly copy constructor . Cheers

Share this post


Link to post
Share on other sites
True.

If you do the same thing with std::vector it works. Well, you have to do it in a special way, but it can work (create the vector with size ). You can''t push_back, though

Another option would be to define the copy constructor.

Cheers

Share this post


Link to post
Share on other sites
quote:
Original post by NextS
If you do the same thing with std::vector it works.

No, *all* STL containers put the same requirements on their elements. That is, they must be copyable and assignable. Failure to observe the semantics of those requirements is a failure on the part of the programmer, not the library. It does *not* work with std::vector.
quote:

Well, you have to do it in a special way, but it can work (create the vector with size ). You can't push_back, though

So, IOW, you mung your code in such a way that it won't call your cctor and reveal that it is broken. Hiding the fact that it's broken doesn't make it not broken. In fact, it's wilfully stupid behaviour. It's not even funny.
quote:

Another option would be to define the copy constructor.

Another option? It's the only option if you are at all concerned with correctness. You also need an assignment operator.

[edited by - SabreMan on November 1, 2002 10:00:12 AM]

Share this post


Link to post
Share on other sites
So what do you think this is all about ? I think this IS funny.

This is only a story about wasting time with stupid bug tracking finding the bug and thinking to yourself: This was so obvious. How could I ever do this wrong.

The code only served as a simple example for how it does not work. It''s not at all the code I use. It''s not even equal to my data structures. Only example. I do know well how the stl containers work. This is just a bug i found myself having programmed (using the this pointer in the same class as you are in makes no sense by no means at all).

Sorry for having bothered you.

Btw. I never said it was a failure on the STL side, did I ?

If you want to know:
I coded a data pool in which several data was stored. This data was stored in a map and referenced by other code. When I inserted into the map, the data was copied and the references to the data in the map became invalid. That''s it.

Sorry for having bothered you again and wasting your time. Just smile and pass by.

This thread is not for to be taken too serious. So please don''t.

Share this post


Link to post
Share on other sites
quote:
Original post by NextS
This is only a story about wasting time with stupid bug tracking finding the bug and thinking to yourself: This was so obvious.

But is it obvious to you? Despite resolving the "problem", you still do not seem to understand the source, and are pinning it on std::map.
quote:

The code only served as a simple example for how it does not work. It's not at all the code I use.

Then I take it back that you are being wilfully stupid. However, you do seem to have some misunderstandings that are worth explaining...
quote:

Btw. I never said it was a failure on the STL side, did I ?

The thread title is "intersting behaviour of std::map". It is nothing to do with std::map, it is to do with the fact that your class has a fault. That fault still exists in the absence of std::map, as the following program demonstrates:


    
#include <iostream>

using namespace std;

class A
{
public:
A(void):mThis(this)
{}
virtual void notify ( void )
{
mThis->callMe ();
}
virtual void callMe ( void )
{
cerr << "A"<<endl;
}

private:
A *mThis;

};

class B: public A
{
public:
void callMe (void)
{
cerr << "B"<<endl;
}
};

int main (int argc, char **argv )
{
B *pb1 = new B;
B *pb2 = new B(*pb1);

pb1->notify();

delete pb1;

pb2->notify();

delete pb2;
}


You would benefit from knowing about the rule of three: if your class requires one of a cctor, assignment operator or dtor, it probably needs all three. In your case, it doesn't actually need a dtor, but does need the other two. The fact that you have a pointer data member should be a strong indicator that you need to review the copy semantics of your class.

[edited by - SabreMan on November 1, 2002 12:10:21 PM]

Share this post


Link to post
Share on other sites
Nice example thank you

Well the thing is that in my broken code the objects weren''t meant to be copied. So even your example (if I had used such a construct) would have produced the mistake as would any use where copy construcor or assignment operator were used (Only example program''s mistake). ( I imagine the wonderful example of returning a functions variable by reference and using it outside the function )

The problem with my code was in fact the copying. It just happened to be the std::map that made the copy. And this was what i did not see in the first place.

But by the way in my concrete problem (not the example) even the assignment operator and cctor wouldn''t have helped me much, because as I stated earlier the std::map served as a pool that gave away references to other classes. If the methods were defined, the copy would have been correctly, but the references spread all over the program were still wrong. Defining the cctor and ass-op private would have let me find the bug much faster

So the improved problem set would be as follows.


  
class A
{
public:
void call(void){cerr << "Hello World!" <<endl; }
};
class B
{
public:
B (A*a):mA(a){}
void callA ( void ){ a->call(); }
private:
A *mA;
};

void func(void)
{
// wrong code

std::map<string,A> aPool;
aPool["A"] = A();
B b(&aPool["A"]);
aPool["B"] = A();
b->callA();

// corrected code

std::map<string,A*> aPool;
aPool["A"] = new A();
B b(aPool["A"]);
aPool["B"] = new A();
b->callA();
}



So to correct my problem the following step did take place:
Store pointers in the std::map (with creation of objects via new ). The default copy constructor and assignment operator as they work are perfectly right here (for class B). Wheras class A may not be copied from the map. But it may be in general. So to find the bug and only for it I might have declared the cctor and ass-op for class A private.

I hope I made it clear with it. My example seems to be wrong chosen and does not map my Problem very well.

I think this happened because when I finally found the bug it was 5am or so. Sorry for that. So in my example vector would have worked correctly as the access function would not have made a copy but just returned a reference I think.
quote:
Original post by NextS
Well the thing is that in my broken code the objects weren't meant to be copied.

I still think you're missing the point. There's two issues here. Firstly, if something is not meant to be copied then you should enforce that by disabling the copy constructor, and probably the assignment op. However, the important point you are missing is that all Standard Library containers make a requirement on element types that they are copyable and assignable. That means you either have to ensure the semantics of those operations are correct for your class, or you have to find a container which does not make those requirements. It sounds to me like you've designed your way into a corner.
quote:

The problem with my code was in fact the copying. It just happened to be the std::map that made the copy. And this was what i did not see in the first place.

Any reasonable Standard Library documentation will tell you this. It's part of a programmers job to ensure they are informed in what they are doing - read the documentation!
quote:

If the methods were defined, the copy would have been correctly, but the references spread all over the program were still wrong.

Then you chose the wrong solution to your problem. But hopefully you've learned something in doing so.

quote:
Original post by daerid
SabreMan, you and DrPizza are like the two forum stupidity guards.

Is that good or bad?

[edited by - SabreMan on November 4, 2002 11:57:53 AM]

Share this post


Link to post
Share on other sites