Preventing memory leaks

[retroCheck]

I have code that uses a vector, multimap,iterators and a struct. In freeing up my memory I have used the delete() for iterators and maps. Is that the proper way(line 160).

Also when I declare a User user struct(line 70) does line 157 completely free the memory. The way I see it I allocated memory for a struct user and keep using the same chunk of memory as I go through my while loop.

...
 12 typedef struct{
 13         char *Name;
 14         int Age;
 15         double W;
 16         double H;
 17 }User;

...
 66 while(fgets(line,200,fp)){
 67         //Scan line and put each space seprated chars into vars.
 68         sscanf(line,"%s %d %lf %lf",nameH,&ageH,&wH,&hH);
 69         //Allocate memory
 70         user = (User *) malloc(sizeof(User));
 71         //Store vars into user structs.
 72         user->Name = strdup(nameH);
 73         user->Age = ageH;
 74         user->W = wH;
 75         user->H = hH;
 76 
...

 89         //Insert into data structures
 90         userV.push_back(user->Name);
 91         hName.insert(pair<double,char*>(user->H,user->Name));
 92         wName.insert(pair<double,char*>(user->W,user->Name));
 93         ageName.insert(pair<int,char*>(user->Age,user->Name));
 94 }
 95 //--------------------------------------------------------------------
 96 //Output Data in specified format.      
 97 
 98         //Declare Iterator variables
 99         multimap<double,char*>::iterator iterhName;
100         multimap<double,char*>::iterator iterwName;
101         multimap<int,char*>::reverse_iterator iterAgeName;
102 
103         //List of names in the same order as the input file.
104         printf("%s\n","====order====");
105         for(int i = 0; i<userV.size(); i++){
106                 printf("%s\n",userV[i]);
...

150         //free up all memory allocated
151         char* e;
152         for(int i = 0; i < userV.size(); i++){
153                 e  = userV[i];
154                 free(e);
155         }
156 
157         free(user->Name);
158 
159         for(iterhName = hName.begin(); iterhName != hName.end(); iterhName++){
160                 delete((*iterhName).second);
161         }
162 
163 
164         for(iterwName = wName.begin(); iterwName != wName.end(); iterwName++){
165                 delete((*iterwName).second);
166         }
167 
168         for(iterAgeName = ageName.begin(); iterAgeName != ageName.end(); iterAgeName++){
169                 delete((*iterwName).second);
170         }

[ne555]

delete is not a function, the parenthesis are not needed.
You must not delete what you didn't new. Don't mix them with malloc() of free() either.


Line 70 is inside a loop, in the next iteration you'll make another iteration. Also, you never free(user)
¿why can't you simply User user;?


line 90--93
They all point to the same memory address. The loop in line 152 would invalidate the pointers of the multimaps.
¿what do you want there? If you erase an element of the vector, ¿do you want it to disappear from the multimaps too?

line 157 is trying to free invalid memory. It's equivalent to free( userV.back() )


By the way, using floating point numbers for keys may give you issues with precision.

[retroCheck]

paragraph 1 - So the more I think about these iterators are holding a pointer to memory that has already been allocated. So if I deallocated that earlier with -

151 char* e;
152 for(int i = 0; i < userV.size(); i++){
153 e = userV[i];
154 free(e);
155 }
...
157 free(user);

I shouldn't need to worry about the iterators or anything else. So does the this code here deallocate everything or does the while loop create several different chunks of memory(line 70) that needs to be deallocated.

paragraph 2 - Not using User user; because this is a C exercise, just using C++ data structures.

paragraph 3 - Just wanted to deallocate the memory. This program will print out data and be finished. Just trying to prevent memory leaks.

paragraph 4 - Because it has been freed in my e= userV...(151 - 155) loop?

[keskiverto]

paragraph 2 - Not using User user; because this is a C exercise, just using C++ data structures.

The C++ "data structures" absolutely require the use of C++, which by definition should not be allowed in a C exercise?

[MiiNiPaa]

Not using User user; because this is a C exercise, just using C++ data structures.

You can have the same in C too.

All other your problems are because you do not get meaning of ownership: in one moment there should be exactly one entity owning pointed value and responsible for it deletion. Other users should either copy value, or use non-owning pointers which should not delete pointed value and should not be used when owner deletes pointer value.

[retroCheck]

@MiiNIPaa So let me see if I understand. A new chunk of memory is created through each iteration of the loop. Before next iteration I need to decide how that user pointer is to be handled(take ownership). Otherwise next iteration I am allocating another chunk and putting another pointer value into the user variable. The chunk of memory is lost at this time if I don't handle it because the address to this memory has not been saved anywhere.

Solution - I have chosen to free(user)at the end of every iteration. I have no need to copy the data. I just need to store certain values from the struct into a data structure. I see now that I didn't even need a struct. I could have just placed values from sscanf() into variable and then put them into data structures. I was thinking in the beginning that I would be storing these these structs into vectors and accessing member variables for print out.

@keskiverto, I understand. I have taken a data structures class. This exercise is to just brush up on C, not build data structures. I suppose building a simple data structure would also be a good way to brush up on C.

[MiiNiPaa]

Only thins which are needed to remove are user structure and names.
I suggest to do:
1) drop using User struct alltogether. You do not really need it.
2) Declare one container as owning all strings it stores. All others will contain only pointers to that strings.
I suggest to use userV vector for it: at the end all entries in vector should be freed. All other containers can be just destroyed as they do not own their strings. Just make sure that nothing uses those strings after vector is freed.

[retroCheck]

ok got it, Thanks again!