Why would vector::size() give a garbage value ?

[kmkkra]

Hello to anyone who reads this, and a very good day to you.

OS: Windows 7
IDE: Code::Blocks
Status: novice C++ programmer

My problem is that myClass.size() is giving a garbage value
when it should be giving the value 2.

My problem is demonstrated below, but I don't know how to replicate the problem exactly. The program below pretty much demonstrates what I'm doing in my actual program. Also note that I checked myClass.empty() immediately after delete, and even though in my original program myClass.size() is giving a garbage value myClass.empty() returns false... So I geuss myClass - isn't - empty, and neither should it be.

And lastly my program crashes once I call myClass.erase()


My questions are:


1)What is wrong with my program?

2) I am expecting to delete the contents that the second pointer element points to. Am I doing my pointer arithmetic wrong?

#include <iostream>
#include "CMyClass.h"
#include <vector>
using namespace std;
 
int main()
{
    vector<CMyClass*> myClass;
    myClass.push_back(new CMyClass());
    myClass.push_back(new CMyClass());
 
    cout << "1) myClass.size() " << myClass.size() << "\n"; //This myClass.size() is cool, it gives 2
 
    vector<CMyClass*>::iterator it = myClass.begin() + 1;// I - think - this equals the second element
    delete *it;
 
    if (myClass.empty()) std::cout << "myClass is empty ya'll\n";
 
   cout << "2) myClass.size() " << myClass.size() << "\n";  //In my original program myClass.size() gives a garbage value
 
    myClass.erase(it);
 
    cout << "3) myClass.size() " << myClass.size();
 
    return 0;
}

As always, thank you so much to anyone who helps, and the best of wishes to all the gurus, and the members on CodeGuru.

[Maejie]

You "delete" it before it is erased by you,

[laserlight]

It looks okay to me (other than the failure to use delete yet another time). My guess is that the problem lies with CMyClass.

[kmkkra]

It looks okay to me (other than the failure to use delete yet another time).

Hi laserlight, what do you mean by "failure to use delete yet another time" I only wanted to delete the second element in this example. Am I not deleting both the second myClass element's allocated memory and the second myClass element correctly?

If not - how should it be done?


-------------------------------

My guess is that the problem lies with CMyClass.

The only thing that I can see that could cause the myClass.size() to return a garbage value(actually it looks like an address) immediately after calling delete *it would be in myClass destructor.

Now, I can see some definite potential for errors in myClass destructor seeing as that CMyClass is part of a depency circle with about two or three other classes, and so therefore these classes are represented as raw pointers with allocated memory declared within each other, and they are deleted within each others destructors.


EDIT: I commented out the delete statements within ALL the destructors, and myClass.size() actually returned the correct value for some reason, ofcourse, this still doesn't solve my problem.


--But what I don't understand is how would all this aforementioned criteria cause the first cout statement, which is -immediately - before the " delete *it ", to print value 2 which is correct:

cout << "1) myClass.size() " << myClass.size() << "\n";

But the second cout statement is -immediately - after the delete *it. The second cout statement is where the garbage/unknown value appears.

I can understand other errors occurring, but do you know why this particular myClass.size() error would occur?



Thanks

[Paul McKenzie]

But the second cout statement is -immediately - after the delete *it. The second cout statement is where the garbage/unknown value appears.

I can understand other errors occurring, but do you know why this particular myClass.size() error would occur?

We have no idea how much corruption of memory your destructor of CMyClass is doing. If it is corrupting memory, then all bets are off as to how the rest of your program will behave.

So if you're using faulty classes to begin with, then the solution is to fix your class so that it has a proper destructor instead of a buggy one. For example, here is std::string* as the type stored in the vector, using a program similar to yours:

#include <string>
#include <vector>
#include <iostream>

using namespace std;

int main()
{
   vector<string*> pVec;
   pVec.push_back( new string() );
   pVec.push_back( new string() );
   cout << pVec.size() << "\n";
   vector<string*>::iterator it = pVec.begin() + 1;
   delete *it;
   pVec.erase( it );
   cout << pVec.size() << "\n";
   delete pVec[0];
}

Output:
2
1

The std::string class is not trivial, and vector<string*> behaves correctly using a program similar to yours. If you copied and pasted the program above, if it shows proper behaviour, then it definitely points to destructor bugs in your classes.

Regards,

Paul McKenzie

[kmkkra]

@ Paul McKenzie


Why did you call delete on the first element of pVec:

delete pVec[0];

?

Does this have something to do with what's causing my error?

If not - then I suppose it may just be memory corruption as you've said, but I tell ya - I've looked at my code, and the only thing that I can see wrong with the delete statements in my destructors is possibly that each of the members, that have been allocated in the constructor and therefore deleted in the destructor, are dependent on each other in a circular fasion.



-----------------------------------------
Could dependencies like I've described (the kind of dependencies that cause incomplete types) cause problems with delete?

Cause they don't seem to be causing problem with the new operator, and I would think they would only cause compile time errors anyway - I think...

[Paul McKenzie]

@ Paul McKenzie


Why did you call delete on the first element of pVec:

delete pVec[0];

Because it was allocated using new, therefore no memory leaks when the program exits.

Does this have something to do with what's causing my error?

No. If I left out that line, then the only issue is a memory leak at the end -- the vector behaves correctly in any event.

I've looked at my code, and the only thing that I can see wrong...

If it's wrong, then it's wrong. Again, improperly handling the free-store in any way will result in your program exhibiting undefined behaviour.

Could dependencies like I've described (the kind of dependencies that cause incomplete types) cause problems with delete?

Why not post your classes? Then we can all see what you're attempting to describe, and more than likely, show you what is wrong with them.

Cause they don't seem to be causing problem with the new operator, and I would think they would only cause compile time errors anyway - I think...

All a compiler can do is tell you if your program is syntactically correct. It does nothing to show you if you're program is logically correct or is handling the free-store and heap correctly.

Note that none of these issues has anything to do with vector -- all vector did was show you that your class has a buggy destructor. You could have just written a plain-vanilla C++ program that just did this:

CMyClass *c1 = new MyClass;
CMyClass *c2 = new MyClass;
delete c2;
delete c1;

and the bug is still there. It's just that there isn't a mechanism in the above code to show you that you have bugs (besides having a program crash). By adding a vector, the bug manifests itself in corrupting the vector demonstrated by the size() returning a bad value.

Regards,

Paul McKenzie

[kmkkra]

Why not post your classes? Then we can all see what you're attempting to describe, and more than likely, show you what is wrong with them.

Ok I'll post a bare bones version .Below is a compressed file with all the potentially relevent code, although, I have stripped each file down to nothing. I somehow have skinned my program down to the essentials to produce my problem. I don't know how I did it because I am not thinking clearly at all ( I have a secvere psycological issue which impedes my thinking).


I think I may have found the problem already. I am inserting new elements to vector::end. At the time of writing the code I just hoped that everything would work, and I didn't see any immediate problems so I let it be.

Q)So, will inserting a new element at vector::end cause a problem like memory corruption?




The problem occurrs in void CDomainControl:estroy(int pos)



UPDATE 8/15/11: this file hase been updated in respect to laserlight post #9
The problem occurs in CDomainView::~CDomainView(). It occurs when I delete CDomainView's member: control.
--------------
8/19/11 WhatsWrongAgain.zip has been removed.

[laserlight]

Ok I'll post a bare bones version .Below is a compressed file with all the potentially relevent code, although, I have stripped each file down to nothing. I somehow have skinned my program down to the essentials to produce my problem.

I took a look at your code and I think you need to strip it down further, leaving out files that you do not need to produce the problem. As you do so, keep a few things in mind:

• Try to make your member variables private.
• Instead of having separate init member functions, use the constructor.
• If you are defining a destructor, and not just to declare it virtual, then you should also define or disable the copy constructor and copy assignment operator.
• Comment your code. Explain the reason for the various classes and (especially public) member functions, e.g., given an overview of they model or do. At the moment you seem to have so many interdependencies that it is hard to trace what could affect what and why.

I think I may have found the problem already. I am inserting new elements to vector::end. At the time of writing the code I just hoped that everything would work, and I didn't see any immediate problems so I let it be.

Q)So, will inserting a new element at vector::end cause a problem like memory corruption?

No, if you do so correctly.

[kmkkra]

I took a look at your code and I think you need to strip it down further, leaving out files that you do not need to produce the problem. As you do so, keep a few things in mind.

I updated my program for you, and stripped it down as you've said. You were right it could be stripped down even further. I also narrowed down what the problem is - which I noted in my last post next to the file up for review.



Thanks and good day...

[Lindley]

The problem occurs in CDomainView::~CDomainView(). It occurs when I delete CDomainView's member: control.

Run this simple program. You should observe the same error. The problem is that your copy constructor is not correct, and you also have not defined operator=.

int main()
{
    CDomainView v1;
    CDomainView v2 = v1;
}

[Paul McKenzie]

I updated my program for you, and stripped it down as you've said. You were right it could be stripped down even further. I also narrowed down what the problem is - which I noted in my last post next to the file up for review.

To add to what Lindley stated, if you really wanted CApplicationView to turn off copying, you did not do it correctly.

1) The copy constructor should be private, which you did do, but it should also be unimplemented, meaning it should not have a function body. Your version has an empty function body, which is not good enough -- it should have no function body at all.

This is the corrected code:

CApplicationView(CApplicationView&);  // note -- no function body, not 
                                                             // even an empty one!

The reason is that if you have any function body whatsoever, you can still make copies within the CApplicationView class itself, and copying is what you're trying to avoid. Making the function unimplemented completely turns off the copying mechanism, both at compile and link time.

2) You did not overload an operator= to turn off assignment. It should be done the same way as the copy constructor -- private and unimplemented.

Regards,

Paul McKenzie

[kmkkra]

Run this simple program. You should observe the same error. The problem is that your copy constructor is not correct, and you also have not defined operator=.

Ok, I added, at least, what I assume is a reasonably correct set of operator='s. I also added a non-const copy constructor. Which is what I am assuming is what you meant by that my constructor isn't correct. I am still getting the same errors though...

I added an updated file to post #8 with these corrections.

I am not sure what more I need to do to my constructors and operator='s to make them any better, but since you informed me that that was the problem then I am sure I will eventually figure it out.

But if you or anyone would like to help, I ,obviously, would be relieved of a little burden.


I also have attemted to make CApplication a singleton. I think some more refinement needs doin' though.



Thanks Lindley

[Paul McKenzie]

Ok, I added, at least, what I assume is a reasonably correct set of operator='s. I also added a non-const copy constructor. Which is what I am assuming is what you meant by that my constructor isn't correct.

All you need is one copy constructor, and that is the const one.

Also, you can't just have "reasonably correct" copy constructor or assignment operator. They must be completely correct. The reason is that copying and assigning are "atomic" operations that are done on instances, either implicitly or explicitly by you or the compiler. You cannot anticipate where a copy will or won't be made, so you must code your copy constructor not knowing when or where it may be called.

This means that in a program, if you make a copy of an object, if you use the original object or the copy, the program must behave exactly the same. For example:

#include <string>
#include <iostream>

int main()
{
   std::string s1 = "abc";
   std::string s2 = s1;
   std::cout << s2;
}

If I replaced s1 in the cout instead of s2, then I better see "abc" outputted. If for example, s2 printed "abczzz", then s2 is not a logical copy of s1.

There is nothing stopping a programmer from making bogus copies using the copy constructor (or assignment operator). However, making bogus copies and using them as if they are the original is one of the toughest bugs to find and fix. Here is a scenario where things go haywire:

void foo(CMyClass x)
{
}

int main()
{
   CMyClass c;
   foo( c );
}

The copy constructor is called, since CMyClass is passed by value to foo. Now if you're making bogus copies, you can imagine the nightmare you have if this is done in a non-trivial program. You have in foo() a parameter x that is a mutation of what was passed, when in reality, it should logically be the same thing as "c".

I am not sure what more I need to do to my constructors and operator='s to make them any better, but since you informed me that that was the problem then I am sure I will eventually figure it out.

The question you should ask yourself is this -- what makes two objects equal? You have static members, you have a vector of pointers, etc. None of these entities have proper copy semantics by themselves. So what makes object 1 equal to object 2? Whatever that is, that is what you code, and it has to be properly done. It's your class, it's your objects, you should know beforehand what makes two objects equal to each other.

As to the problem, the issue seems to be this:

void CDomainControl::destroy(int pos)
//...
  delete *it;

You should note that this happens to be the same pointer that it->control is pointing to. So you wind up deleting yourself when you delete *it.

On a high level, what exactly are you trying to accomplish? In the current code, you tie yourself up in knots deleting yourself. Maybe once we know what you're really trying to do, a better design than what you have now can be suggested.

Regards,

Paul McKenzie

[kmkkra]

The question you should ask yourself is this -- what makes two objects equal? You have static members, you have a vector
of pointers, etc. None of these entities have proper copy semantics by themselves. So what makes object 1 equal to object
2? Whatever that is, that is what you code, and it has to be properly done. It's your class, it's your objects, you should
know beforehand what makes two objects equal to each other.

As to the problem, the issue seems to be this:

void CDomainControl::destroy(int pos)
//...
  delete *it;

You should note that this happens to be the same pointer that it->control is pointing to. So you wind up deleting yourself
when you delete *it.

Ok, now I think I understand. I will have to take a little while to digest everything, but I think I understand.

You know, I have rewritten this program 2 previous times already trying to make it a better design, and I had the exact
same problem with my previous design. And I solved it by using " delete this; " within void
CDomainControl:estroy(int pos) and I didn't call delete control from within CDomain's destructor. I called " delete this
" at the very end of CDomainControl:estroy(int pos). As a matter of fact I was aware of this very current issue at one
point, and then I just forgot about it.

MY problem is that I have a psycological ailment plaguing me. For me, thinking is like driving a car with no tires or
like seeing the world or even my own thoughts through a straw. So this makes complex thought very difficult, and very very
very very slow and tedious. Excuse me while I scream within myself..........................

On a high level, what exactly are you trying to accomplish? In the current code, you tie yourself up in knots deleting
yourself. Maybe once we know what you're really trying to do, a better design than what you have now.

Actually, I was just going to ask you guys for adivice about what design I should use.

I read in wikipedia that the observer pattern would be appropriate (I haven't thoroughly studied this pattern yet), and
don't know how to apply it to my current paradigm which is...

=============
I understand that programs and classes within programs are meant to model the real world. So my design is supposed to be
just that.
My program is divided into for major parts:
1)APPLICATION
2)DOMAIN
3)ORDER DISPLAY WINDOW
4)ORDER PREFERENCE AREA
----------------

Each part is expressed as a class and then I further subdivide the classes by graphical aspects, control aspects, and data aspects( something like the view, model, control paradigm)
===========



On screen, that is gui, the ORDER PREFERENCE AREA, and ORDER DISPLAY WINDOW are contained within the DOMAIN, and the DOMAIN(s) is presented contained within the APPLICATION. So, logically, I tried to model these aspects within my design. I don't know what design
a professional would choose or how they would divide up their program so I just did my best and hoped for the best.

So what design should I use?
What advice can you give?



Thank you so much for your help Paul McKenzie, laserlight,and Lindley. Note: These "thank yous" will continue and they truly
are not cliche or disingenuousness. I think you know that these forums are nothing without you guys.