Why is my Vector Iterator Not Working as Expected in C++?

[exitwound]

I'm trying to work on a project for class (that's not homework but a long-term assignment, second c++ semester so I'm not very bright yet) and I'm having trouble figuring out why the iterator of this vector won't work as I hoped.

Here's the structure of the vector(s) so far:

CarList is a vector<Auto>.
carToModify is the element of the CarList vector I'm working on.

Auto is a class with the following:
string carName;
string carPurchaseDate;
double carCost;
string randomUID;
vector<Part> PartsList;

Part is a class that has the same type of information (name, purchase date etc) but I don't think that's relevant to this.

I display the list of parts on the car and I want to delete one of them (say, the one at the first position in PartsList vector (index=0)). In order to do that, I need an iterator pointing to the first position in that PartsList vector, correct?

vector<Part>::iterator it = (carList[carToModify].Get_Parts_List()).begin();

.Get_Parts_List() returns PartsList.

And once I have the iterator, I can use .erase(iterator) to erase that value in the vector, correct? (subsequently shrinking the vector down by 1 and shifting the indices of the other parts).

(carList[carToModify].Get_Parts_List()).erase(it);

I get vector out of bounds errors when I do this though.

Can you help?

 

[rcgldr]

Vector in a class. After a rethink on this it shouldn't be an issue. I'm pretty sure it's implemented as a pointer to a structure with info about the actual instance of a vector.

 

[exitwound]

Hmm I never thought of doing that... I'd have to do a bunch of fixes to the project but it never crossed my mind to do that.

Why is having the vector inside a class an issue?

 

[xiss burg]

Are you sure you pushed_back a few elements inside the vector? And I see no reason why having a std::vector inside a class would be a problem, makes no sense. There are a few requirements for the std::vector element type, IIRC, it must be default constructable, copy constructable and assignable (operator = ).

 

[exitwound]

Indeed. There is at least 1 item in the vector. The item has all of the appropriate values stored as well. I can confirm this through Visual Studio Stepping-Thru the push_backs.

 

[rcgldr]

oops, see next post.

 

[rcgldr]

Are you sure Get_Parts_List() is working? What happens if you try:

vector<Part>::iterator it = carList[carToModify].PartsList.begin();

carList[carToModify].PartsList.erase(it);

 

[D H]

.Get_Parts_List() returns PartsList.

Does this method return PartsList, or a reference to PartsList? There is a big difference between the two. This is what I am talking about:

vector<Part> Auto::Get_Parts_List () {return PartsList; }

versus

vector<Part> & Auto::Get_Parts_List () {return PartsList; }

The first will make a new vector. Each element in this new vector will be a copy of the corresponding element in the PartsList data member of an Auto object. The take-away point is that you get a new vector each and every time you call this method. If you operate on one of the elements in this new vector you will not change any of the Auto object's Parts.

The second returns a reference to the PartsList in the Auto. Operate on an element of the value returned by this method and you will change one of the Auto object's Parts. The reference is a pseudonym for the Auto object's PartsList data member. Call this method multiple times for the same Auto object and you will get the same reference each time.

 

[exitwound]

That was exactly the problem. I wasn't Getting the Parts List by reference. It now grabs the list properly and passes it around perfectly.

Thanks for the help! I KNEW I was doing the iteration properly (or at least had the right idea).

 

[rcgldr]

vector<Part> & Auto::Get_Parts_List () {return PartsList; }

Returns a reference to the PartsList in Auto.

I figured it was related to Get_Parts_List(), but I wasn't sure. Thanks for the help DH.

So how long did it take you people to learn C/C++/C#?

Still learning, and now my brain hurts.

 

[jtbell]

From a design point of view, wouldn't it be better to make this "part-deleting" action a member function of class Auto? The prototype would look something like this:

void Auto::RemovePart (int partID);

where partID identifies the part you want to remove. It could be either the position in the part list (which is what you have now), or a catalog number or similar ID that is stored in an object of class Part. Then to remove a part, you would use this member function call:

carList[carToModify].RemovePart(partID);

A member function of Auto can act on the PartsList vector directly, without having to pass it around and raise the possibility of an error like the one we saw here.

 

[D H]

I agree. I have seen on multiple occasions classes in which all data members are declared private but each data member has a publicly visible (simple) getter (non-const getter!) and setter. What's the point? Making those data private doesn't accomplish much, if anything.

Information hiding entails a lot more than just making the data protected or private.

 

[rcgldr]

vector<Part> Auto::Get_Parts_List () {return PartsList; }

versus

vector<Part> & Auto::Get_Parts_List () {return PartsList; }

The first will make a new vector.

I just checked this using Visual Studio 2005. It's not a "new" vector but instead local instance of the class on the stack.

 

[D H]

I meant new as in different, Jeff, and it most certainly is "new" in that sense. It is allocated and populated via the vector assignment operator. It is not new in the sense of something that must be deleted by the user. (How can it be? It's not a pointer.)

class Auto {
public:
  vector<Part> Get_Parts_List () { return PartsList;}
private:
  vector<Part> PartsList;

...
};

void check_auto () {
   Auto auto;
  vector<Part> parts_list = auto.Get_Parts_List();
...
}

In the function check_auto if you print the address of parts_list and auto.PartsList you will see that they are completely different vectors. You will have to do this in the debugger because auto.PartsList is private in my example code.

 

[Hurkyl]

What's the point? Making those data private doesn't accomplish much, if anything.

It doesn't accomplish anything if the class is designed absolutely perfectly, will never change, and there will never be a situation where it and some other class need to offer the same interface.

(actually, that's not true -- it does make the interface more uniform)

However, it's not always a good idea to assume those properties are satisfied.

 

[rcgldr]

In the function check_auto if you print the address of parts_list and auto.PartsList you will see that they are completely different vectors.

I was trying to follow what the VS2005 is doing with vector memory allocation in the case of this coding bug (Get_Parts_List returning a copy instead of a reference). What's a bit confusing is that VS2005 pre-allocates stack space at compile time based on the number instances of Get_Parts_List() in source code, and each instance in the source gets a different address, some compile time fixed offset on the stack. If a single instance of Get_Parts_List() is in a loop, it gets the same address for each iteration of the loop. Any data that is allocated for the vector would get lost on the next iteration of Get_Parts_List(), since it recopies the primary structure each iteration.

 

[D H]

However, it's not always a good idea to assume those properties are satisfied.

A lot of IDEs provide the ability to automatically create simple setters and getters. Change the names of the underlying member data and those setters and getters will either vanish or automagically change their names correspondingly, depending on the IDE.

When people do this kind of stuff without the aid of an IDE (make a setter and getter for each data member), they tend to change the public interface by hand if they decide to rename the member data.

Anyhow, harkening back to jtbell's comment, information hiding when done right involves a lot more than just making ones data private and providing setters and getters for them. I was using the example of a class with a simple setter and getter for each data member as an example of information hiding done wrong in my opinion -- and I have voiced this opinion during code walkthroughs.

I was trying to follow what the VS2005 is doing with vector memory allocation in the case of this coding bug (Get_Parts_List returning a copy instead of a reference). What's a bit confusing is that VS2005 pre-allocates stack space at compile time based on the number instances of Get_Parts_List() in source code, and each instance in the source gets a different address, some compile time fixed offset on the stack.

That's no different from the treatment of any other local (auto) variable. Most compilers allocate such objects from the stack (as opposed to the heap for new). Consider two cases, one in which Get_Parts_List() returns a vector<Parts> and the other in which the method returns a vector<Parts> &:

• Returned value and local variable is of type vector<Parts>.
  Assuming a statement vector<Parts> parts_list = auto.Get_Parts_List(), the compiled code will
  • Allocate memory of size sizeof(vector<Parts>) for the variable,
  • Construct this allocated memory via the vector<Parts> default constructor, and
  • Copy the auto.PartsList to parts_list via the vector<Parts> assignment operator. There will be yet more allocation, construction, and copying if the vector is not empty, one for each of the elements of the vector. Here the allocation is from the heap, not the stack.
• Returned value and local variable is of type vector<Parts> &.
  Assuming a statement vector<Parts> & parts_list = auto.Get_Parts_List(), the compiled code will
  • Allocate memory of the size of a pointer for the variable parts_list from the stack and
  • Set the contents of that memory to the address of the auto.PartsList data member.

Note that the allocation is quite simple in the latter case; all that is allocated is enough storage for a pointer. No copying, etc. is needed because this is a reference to an existing vector rather than a brand new vector.

 

[rcgldr]

Consider two cases ...

Comments added to list explaining what VS2005 (Visual Studio 2005) does:

Get_Parts_List() returns a vector<Parts>:

• Returned value and local variable is of type vector<Parts>.
  Assuming a statement vector<Parts> parts_list = auto.Get_Parts_List(), the compiled code will
  • Allocate memory of size sizeof(vector<Parts>) for the variable,
    • VS2005 - pre-allocates memory for temporary instance of vector<Parts> on the local stack at compile time.
  • Construct this allocated memory via the vector<Parts> default constructor, and
  • Copy the auto.PartsList to parts_list via the vector<Parts> assignment operator. There will be yet more allocation, construction, and copying if the vector is not empty, one for each of the elements of the vector. Here the allocation is from the heap, not the stack.
    • VS2005 - destroys the copy after it's used. Any saved pointer would point to freed memory.

Get_Parts_List() returns a vector<Parts> &:

• Returned value and local variable is of type vector<Parts> &.
  Assuming a statement vector<Parts> & parts_list = auto.Get_Parts_List(), the compiled code will
  • Allocate memory of the size of a pointer for the variable parts_list from the stack and
    • VS2005 - Get_Parts_List() returns the pointer in a register, EAX. Elsewhere the pointer is a pre-allocated local variable.
  • Set the contents of that memory to the address of the auto.PartsList data member.