Deallocate strdup in std::map or std::vector

[lo2]

Hi there,

So I have some function that puts a lot of char* into a std::map or std::vector

So the function would be something like:

someStruct readFile(const char* fname){
FILE* pFile;
int LENS = 1000000;
char buffer [LENS];
const char *delims = "\t \n";
pFile = fopen (fname , "r");
if (pFile == NULL) perror ("Error opening file");
std::vector<char*> tmp;
while(1){
if (fgets(buffer,LENS, pFile) == NULL) break;
tmp.push_back(strdup(buffer));
}
(Reads data from std::vector into some struct)
for(int i=0; i<tmp.size(); i++){
free(tmp[i]);
}
return(struct);
}

Then at the end I would free all elements of tmp,
also having strdupped them into my struct if I needed any char* there.
I of course also free these, when I am done with my struct

But it seems I get memory leakage anyway,
so could you tell me what I might be doing wrong?

#####

This could also be a std::map, where I would do something like:
Reading the first string from each line of the vector into a map.

someMap[strdup(strtok(tmp[0],*delims)]=1;

How do I then free the space malloced by strdup, I also get memory leakage here? How do I access the keys of a map, or is this just bad practice strdup keys of a std::map?

And how bad is it to have memory leakage, if the elements you do not deallocate, are only tried deallocated in the code in the very end of your program (like last lines of main())?

I hope I made myself clear, if not please ask :)

[JLBorges]

> So I have some function that puts a lot of char* into a std::map or std::vector
> But it seems I get memory leakage anyway,
> so could you tell me what I might be doing wrong?

The fundamental error is in not using RAII, not using std::string, not letting the standard library take care of resource management.
https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization
https://cal-linux.com/tutorials/strings.html

std::vector<std::string> get_lines( const std::string& fname )
{
    std::vector<std::string> lines ;

    std::ifstream file(fname) ;
    std::string a_line ;
    while( std::getline( file, a_line ) ) lines.push_back(a_line) ;

    return lines ;
}

> someMap[strdup(strtok(tmp[0],*delims)]=1;

Again, use std::string

std::vector<std::string> split( std::string str, const std::string& delimiters )
{
   std::vector<std::string> tokens ;

   std::size_t from = 0 ;
   std::size_t pos = str.find_first_of( delimiters, 0 ) ;
   while( pos != std::string::npos )
   {
       if( pos != from ) tokens.push_back( str.substr( from, pos-from ) ) ;
       from = pos+1 ;
       pos = str.find_first_of( delimiters, from ) ;
   }
   if( from < str.size() ) tokens.push_back( str.substr(from) ) ;

   return tokens ;
}

int main()
{
    const auto lines = get_lines( __FILE__ ) ;

    std::map< std::string, int > map ;
    for( std::string str : lines )
    {
        static const std::string delimiters = " \t\n.'\"(){};=!+-,&<>[]#:\\/" ;
        for( auto token : split( str, delimiters ) ) ++map[token] ;
    }
}

http://coliru.stacked-crooked.com/a/5885097785defbab
http://rextester.com/JTGQ30410

[lo2]

Hi and thanks a lot for the response!

I am a bit curious if everyone in the C++ community shares your opinion on this matter?

And is there not a case to be made for using char* instead, in terms of time-wise performance?

[Cubbi]

I am a bit curious if everyone in the C++ community shares your opinion on this matter?

The C++ community is fairly diverse, I am sure some of the ~4 million C++ programmers don't share JLBorges's opinion, but as a personal POV, your code would fail code review at every place I worked at so far, for multiple reasons, some of which would be the use of strtok, strdup, manipulation of owning raw pointers, and representation of strings as raw pointers.

And is there not a case to be made for using char* instead, in terms of time-wise performance?

I would make the case that using char* is bad for "time-wise performance", but it isn't the biggest problem of such code.

[Albatross]

There is not a case to be made for using a raw char* to improve performance. Memory usage is another matter, but for desktop programming, you're nowhere near the point where you need to start being careful. That said, yes, memory leaks are a bad thing.

With C++17 right around the corner, there will soon be precisely one reason to use C-style strings, and that's for interfacing with C code. Then, we'll have a standardized non-owning string reference, namely std::string_view, which takes care of one of the only other reasons you might want to use a char* for your strings. Until then, there is the Boost equivalent, boost::string_ref. If you don't want to use Boost, you can still pass around pointers or references to std::strings.
http://en.cppreference.com/w/cpp/string/basic_string_view
http://www.boost.org/doc/libs/1_63_0/libs/utility/doc/html/string_ref.html

And yes, the supermajority of knowledgeable C++ programmers will recommend to you the use of std::string, especially in container classes like std::vector or std::map.

-Albatross

[dhayden]

And is there not a case to be made for using char* instead, in terms of time-wise performance?

"Premature Optimization is the root of all evil" - Donald Knuth
"The greatest optimization is going from 'not working' to 'working.'" - Ron Hiller
"If correctness doesn't matter then a program can be optimized to an arbitrary degree." - Chris Macey

Okay, I'm sure nobody here has heard of my former coworkers Ron and Chris, but their words are still very valid.

Is there a case to use char* to improve performance? Mmmmmmmaybe... If all else has failed. It will also reduce the memory footprint slightly, but it might actually harm performance. In both cases, that's because std::string stores the size of the string. Do you have a measured performance problem in this code? Are you certain that you need to fix it? Can you improve the performance of some other code instead?

Your readFile() function doesn't appear to leak memory to me, but it does leak the FILE pointer. You need to add fclose(pFile).

Can you use char* as the values in a vector like you have? I suppose so, as long as you keep it crystal clear who owns the char*. For example, don't make a copy of that vector.

Can you use char* as the key in a map? I suspect not, or at least not without difficulty. Before destroying the map, you'd have to remove each item and delete the key after removing it.

So in both of these cases, I'd just use std::string. Doing it with char* will result in fragile code - code that can easily break if someone makes a change to it.

[AbstractionAnon]

Since you're reading from a file, you're going to be spending most of your time in I/O routines. The difference in performance between std::string and C strings is going to be negligible compared to the amount of time you're spending in I/O routines. So the performance argument is nonsense.

You're also going to be spending far more time ensuring the correctness of your program using C strings than if using std::string.

[lo2]

Ok thanks a lot for all the replies :) I will try and implement your suggestions in my code!

Oh and one last thing, was is the best way to convert between char* and std::string?

[Peter87]

lo2 wrote:

was is the best way to convert between char* and std::string?

You can use the c_str() member function to get a const char*.
http://www.cplusplus.com/reference/string/string/c_str/

If you must have a non-const char* you can do &str.front() or &str[0].

[lo2]

I have question, and it is not because I want to go back to using char* instead std::string.

I just do not understand this, and would therefore like your input:

So what would happen if I declare a static char array:

static char str2[40];

And then strcpy my char* from strtok for instance into that.
And then insert that one into my std::map?

Should that not work? And since it is static, should this not work if the function returns this std::map and uses it outside the function?

[AbstractionAnon]

That would declare one array of 40 characters. If you're using strtok(), that implies you're parsing your way through a longer char array and doing multiple strtok/strdup operations.

You were not clear on the full type of your map, but from previous posts, I'm assuming it's std::map<char *,char*>. If you strdup something to str2, then insert str2 into a map<char *,char *>, you're inserting a char pointer (to str2) into the map element. If you subsequently change str2 by another strtok/strdup, you've changed the contents of the str2 array, but you have not changed the pointers (to str2) that were inserted into the map.

[lo2]

Might I ask more general question as to the matter of memory leaks.

And thanks for all the great feedback in here :)

So as I understand a memory leak, if you for instance use malloc, it means that a block the code reserves a block of memory and if this is not properly freed, then it is neither freed to the system.
And thereby a bit of the RAM is just occupied.

So is this correctly understood?
And how come the program does not free such blocks, when it closes?
And would you theoretically be able to "clog up" the RAM doing this repeatedly? And how would you solve it then, reboot the whole system?

[MikeyBoy]

So as I understand a memory leak, if you for instance use malloc, it means that a block the code reserves a block of memory and if this is not properly freed, then it is neither freed to the system.
And thereby a bit of the RAM is just occupied.

That's correct.

And how come the program does not free such blocks, when it closes?

It does.

[lo2]

Ok, well then it is not that big of a problem for small "script like" C++ programs then?

As they do not run for very long.

[AbstractionAnon]

Ok, well then it is not that big of a problem for small "script like" C++ programs then?

Failure to release memory is a bad habit to get into.

It becomes critical when you write larger programs where you are allocating many temporary objects which may have different life spans. Failure to release objects can cause your program to run out of memory.

[Cubbi]

Just calling malloc (or new/new[] or strdup as in the original post) is already a bad habit to get into

[lo2]

Yes ok I see thanks for info :)

[mbozzi]

>And how come the program does not free such blocks, when it closes?
It does.

No, it doesn't. Not if you don't free it.
On devices with fully-featured systems, resources not freed by the application are reclaimed by the operating system when the process dies.

This is not a guarantee made by the language or your program, but rather by the system, and you shouldn't rely on it.

On systems which do not reclaim un-freed resources, the only way for an end-user to release the memory is to reboot the system.

[MikeyBoy]

OK, fair point - it's the OS that reclaims the memory rather than it being the C++ app that releases it. Pragmatically, though, are there any commonly used OS's where this doesn't happen?

[JLBorges]

In practise, it happens in every OS for programs running in the kernel; the most common example of this is a device driver program.