STL vector::erase (iterator) erases the last element

Started by
9 comments, last by Kylotan 15 years, 10 months ago
Hello, Here is a part of my code:
char AARemove (char *alias) {
	vector<AALIAS>::iterator it;
	AALIAS aa;

	if (alias != NULL) {
		aa.Key = alias;
		
		it = find (AAliases.begin (), AAliases.end (), aa);
		
		printf ("Removing \"%s\" - \"%s\"\n", alias, it->Key.c_str ());

		if (it != AAliases.end ()) {
			// Remove it from the aliases list
			printf ("Done: \"%s\"\n", AAliases.erase (it)->Key.c_str ());
			
			return 2;
		}
		return 1;
	}
	return 0;
}


Where vector<AALIAS> AAliases; The AALIAS class looks like this:
class AALIAS {
	public:
		string Key;
		string Command;

		bool operator<(const AALIAS &a) const;
		bool operator==(const AALIAS &a) const;
		AALIAS operator=(const AALIAS &a) const;

		bool Set (char *cmd);
		bool Add (char *cmd);
		void Clear ();
};


Its operators:
bool AALIAS::operator<(const AALIAS &a) const {
	return (Key < a.Key);
}

bool AALIAS::operator==(const AALIAS &a) const {
	return (Key == a.Key);
}

AALIAS AALIAS::operator=(const AALIAS &a) const {
	AALIAS aa;
	aa.Key = a.Key;
	aa.Command = a.Command;
	return aa;
}


AAliases vector contains the following: "cli" "cast lightning" "ccl" "cast continual" "ccf" "cast create" "ccs" "cast create spring" "cac" "cast acid" "css" "cast stone" "cl" "cast cure light" "cb" "cast blindness" "cgs" "cast giant" "cs" "cast shield" Where the first strings are the Keys and the second are Commands. Now the problem is that when called with argument "cac", the function AARemove prints out this: Removing "cac" - "cac" Done: "cac" Which clearly indicates that the entry with Key "cac" wasn't removed. Furthermore, for some reason it removed the last element of the AAliases vector. However, when it->Key.c_str () is equal to "cac", then the iterator just can't be pointing to the last element, can it? It seems that I'm doing something wrong, as according to the documentation, vector::erase shouldn't behave that way.. All suggestions are welcome, Thank You [Edited by - IndrekSnt on May 26, 2008 8:25:21 AM]
Eyes - fountains running blood......That''s me
Advertisement
I see two problems:

(1) AALIAS operator=(const AALIAS &a) const, Why is your assignment operator a const member function? It modifies the object, does it not?

(2) The return value from std::vector::erase() is the element after the last element erased.
Thank You for Your reply..

1) I agree, the assignment operator should not be const. I now changed it.
However, it still erases the last element.

2) If I do vector::erase (it), and 'it' points to "cac", then it shouldn't
point to "cac" after erasing it. It still points to "cac" even after changing
the assignment operator..

All suggestions are welcome,
Thank You
Eyes - fountains running blood......That''s me
Quote:Original post by fpsgamer
I see two problems:

(1) AALIAS operator=(const AALIAS &a) const, Why is your assignment operator a const member function? It modifies the object, does it not?

(2) The return value from std::vector::erase() is the element after the last element erased.



(1) Further more, I believe the assignment function should be better looking like this: Type& Type::operator=(const Type& object)

/*----------------------------------------------------------------------------------------------------------------------------------*/Enthusiastic and wild about game development. Any opportunity would get me sink into any fantastic game-revolution era.
Quote:Original post by IndrekSnt
Thank You for Your reply..

1) I agree, the assignment operator should not be const. I now changed it.
However, it still erases the last element.

2) If I do vector::erase (it), and 'it' points to "cac", then it shouldn't
point to "cac" after erasing it. It still points to "cac" even after changing
the assignment operator..

All suggestions are welcome,
Thank You


I think You probably need to assign the iterator back iter = yourVectorName.erase(iter);
/*----------------------------------------------------------------------------------------------------------------------------------*/Enthusiastic and wild about game development. Any opportunity would get me sink into any fantastic game-revolution era.
Quote:Original post by IndrekSnt
I agree, the assignment operator should not be const. I now changed it.


Can you post the new definition?
AALIAS operator=(const AALIAS &a);

AALIAS AALIAS::operator=(const AALIAS &a) {	AALIAS aa;	aa.Key = a.Key;	aa.Command = a.Command;	return aa;}


@daviddiligent: Sry, my sentence lost its meaning. What I ment was that vector::erase (it) returns an iterator to "cac".

Thank You
Eyes - fountains running blood......That''s me
Quote:Original post by IndrekSnt
Thank You for Your reply..

1) I agree, the assignment operator should not be const. I now changed it.
However, it still erases the last element.

2) If I do vector::erase (it), and 'it' points to "cac", then it shouldn't
point to "cac" after erasing it. It still points to "cac" even after changing
the assignment operator..

All suggestions are welcome,
Thank You


I have questions here.
1> How do you know that it erased the last element? 2> How do you make the iter points to "cac".
If it always erased the last element, doesn't that mean the find() only returns the iterator to the last element - 1? If it did, how did you make your iter points to "cac" ?

By the way, I think the better version of the "=" would be:
AALIAS& AALIAS::operator=(const AALIAS& ob){  key = ob.key;  command = ob.command;  return *this;}
/*----------------------------------------------------------------------------------------------------------------------------------*/Enthusiastic and wild about game development. Any opportunity would get me sink into any fantastic game-revolution era.
Thats it!

The problem really was in that assignment operator.

Thank You all..
Eyes - fountains running blood......That''s me
By the way, if that is all your operator= does, you don't need to write one yourself at all. The compiler would create one that does the same for you automatically, except without errors :)

This topic is closed to new replies.

Advertisement