{ Note: this article is cross-posted to comp.lang.c++,
microsoft.public.vc.stl, gnu.g++.help and comp.lang.c++.moderated. -mod }
Rakesh wrote:
> What is wrong this implementation? I get a core dump at the free()
> statement? Thanks
> #include <ext/hash_map>
> #include <iostream.h>
> #include <ext/hash_set>
> using namespace std;
> using namespace __gnu_cxx;
> struct eqstr
> {
> bool operator()(char* s1, char* s2) const
> {
> return strcmp(s1, s2) == 0;
> }
> };
> int main()
> {
>
> char *s, *s1, *temp;
> hash_set<char *, hash<char *>, eqstr> AddedPhrases;
> hash_set<char*, hash<char*>, eqstr> ::iterator Iter1;
> s = (char *)malloc(sizeof(char)*100);
> strcpy(s, "apple");
> AddedPhrases.insert(s);
> s1 = (char *)malloc(sizeof(char)*100);
> strcpy(s1, "absent");
> AddedPhrases.insert(s1);
> for (Iter1 = AddedPhrases.begin(); Iter1 != AddedPhrases.end();
> Iter1++)
> {
> temp = *Iter1;
> //printf("\nDeleting:%s:%d", temp, strlen(temp));
> free(temp);
I'd be very surprised that this doesn't result in undefined
behavior. You're leaving an invalide pointer in the table.
You're in for some very unplaisant surprises the next time some
other value hashes to this value, and the implementation tries
invoking your compare function on the entry. (More generally,
changing anything which affects the value of anything used for
actually keying causes undefined behavior.)
In fact, a quick analysis of the g++ code in the library shows
that it does re-calculate the hash code in the ++ operator.
(The actual implementation seems to be more space optimized than
performance optimized---although space optimizing the iterator
does mean that it copies a lot faster.) The result is that
anything can happen. You might actually iterator through the
loop only once, or you might iterator more times than there are
elements in the loop, or you might loop endlessly over the same
element, or core dump in the iterator, or ...
The correct way to clean up a container like this would be
something like:
__gnu_cxx::hash_set::iterator i = AddedPhrases.begin() ;
while ( i != AddedPhrases.end() ) {
char* tmp = *i ;
i = AddedPhrases.erase( i ) ;
free( tmp ) ;
}
> }
> }
More generally, of course, you'd be better off:
-- Using the standard IO (<iostream>, <ostream>); g++ definitly
supports it, and the only justification today to use
<iostream.h> is to support legacy compilers (g++ pre-3.0,
Sun CC 4.2, etc.---all so old you shouldn't be using them
anyway).
-- Using std::string, instead of the char* junk. Do that, and
the destructor of the hash_set will clean up everything
automatically.
-- Not declaring variables until you can correctly initialize
them.
--
James Kanze (Gabi Software) email: [EMAIL PROTECTED]
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
_______________________________________________
help-gplusplus mailing list
help-gplusplus@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gplusplus
