Memory release?

[Andreik]

I have this code to get all prefixes of a given string:

#include "stdafx.h"
#include <iostream>
#include <conio.h>

using namespace std;

char * stringmid(char * string, size_t offset, size_t lenght)
{
    size_t max = strlen(string) - offset;
    if (lenght > max) lenght = max;
    char * result = (char *) malloc(lenght + 1);
    if (result)
    {
        memcpy(result,string + offset,lenght);
        result[lenght] = '\0';
    }
    return result;
}

int main()
{
    char s[20];
    cout << "Input string: ";
    gets_s(s);
    for(int i=0; i < strlen(s); i++) {cout << stringmid(s,0,i+1) << endl;}
    _getch();
    return 0;
}

It works pretty well but I'm not sure if I should release or not the memory block allocated with malloc in stringmid function. If so, how can I write a correct stringmid function. Thanks!

[danielkza]

If you freed the result string the callee would have nothing to work with. It's the callee's job to determine the lifespan of the created string and dispose of it accordingly.

And I notice a bug: the passed offset may be larger than the length of the string, which would cause the result of the max calculation to be negative, which cannot be stored in an unsigned size_t, leading to a wrap to what is possibly a very large number (close to UINT_MAX, which is about 4 billion), which is definitely will cause unexpected behavior. Fixing that is actually not that hard: you could either compare the string length and the offset and fail early, or make max signed and check if it's actually non-negative before continuing.

Also, the proper spelling is 'length'

[Richard Robertson]

You should actually work it so that the caller passes both the source and destination buffers. This allows the caller to exactly understand the lifetime and scope of the results.

[Mat]

I asked this question on stacoverflow a year ago: http://stackoverflow.com/questions/8604633/correct-way-return-a-string-from-a-function

[Andreik]

Thank you guys.

Finally I think I got something like Richard Robertson said.

#include "stdafx.h"
#include <iostream>
#include <conio.h>

using namespace std;

void stringmid(char * string, char * strmid, size_t offset, size_t lenght)
{
    if (offset < strlen(string))
    {
        size_t max = strlen(string) - offset;
        if (lenght > max) lenght = max;
        memcpy(strmid,string + offset,lenght);
        strmid[lenght] = '0';
    }
}

int main()
{
    char string[20];
    cout << "Input string: ";
    gets_s(string);
    for(int i=0; i < static_cast<int>(strlen(string)); i++)
    {
        char * strmid = (char *) malloc(i + 2);
        stringmid(string,strmid,0,i+1);
        cout << strmid << endl;
        free(strmid);
    }
    _getch();
    return 0;
}

[danielkza]

You should not call strlen twice for the same string if it wasn't change in between: each call will walk the whole string counting the length, and there's no need to do that twice (you repeat that same mistake on your sample usage code). Cache the result in a variable instead.

Also, when the offset is larger than the string length you should store an empty string or return some kind of error code, otherwise code that tries to reuse the same memory for multiple calls will wrongly re-use the result of previous operations in some cases.

Finally, why the static_cast on the foor loop? It's completely unecessary.

[Andreik]

I cannot return anything because the function is declared as void, but I can store an empty string. Static_cast in the loop it's no big deal, I use it because strlen return a size_t and I got some annoying warning with VC++ 2010 Express. In order to do that I could change data type of variable i.

expandcollapse popup

#include "stdafx.h"
#include <iostream>
#include <conio.h>

using namespace std;

void stringmid(char * string, char * strmid, size_t offset, size_t lenght)
{
    size_t s_len = strlen(string);
    if (offset < s_len)
    {
        size_t max = s_len - offset;
        if (lenght > max) lenght = max;
        memcpy(strmid,string + offset,lenght);
        strmid[lenght] = '0';
    }
    else
    {
        strmid[0] = '0';
    }
}

int main()
{
    char string[20];
    cout << "Input string: ";
    gets_s(string);
    for(int i=0; i < static_cast<int>(strlen(string)); i++)
    {
        char * strmid = (char *) malloc(i + 2);
        stringmid(string,strmid,0,i+1);
        cout << strmid << endl;
        free(strmid);
    }
    _getch();
    return 0;
}

It's better now?

Edited October 11, 2012 by Andreik

[Mat]

It's better now?

Not Quite...

strmid[lenght]

[Andreik]

@Mat I don't get it , can you point me in the right direction?

[Mat]

Typo in leng*th*.

[Andreik]

Oups! It's night here.Thanks. muttley

[Shaggi]

Since you are using c++, are there any reason you are not using std::string, which was designed specifically to eliminate this problem?

[Andreik]

Nope, it's not a special reason, strings are cool. I was interested to learn this thing about memory allocation. This is just an example, I won't use it anywhere.

Edited October 12, 2012 by Andreik

[Shaggi]

Nope, it's not a special reason, strings are cool. I was interested to learn this think about memory allocation. This is just an example, I won't use it anywhere.

alright in c++ though you would typically solve this with use of auto_ptr's.

[Andreik]

I read now about auto_ptr's but I didn't used them never. Can you give me an example or rewrite my example using auto_ptr's?

[Shaggi]

I read now about auto_ptr's but I didn't used them never. Can you give me an example or rewrite my example using auto_ptr's?

I must have been tired or something yesterday, the thing about auto_ptr's is that they do not apply for arrays - like char arrays, because of the difference between delete and delete[] (auto_ptr uses the former). Your example could have been rewritten if you use an object that encapsulates the array... but then you basically created a string anyway.

auto_ptr's are great because of how c++ guarantees that destructors get called. on ~auto_ptr(), it frees it's element; therefore it's pretty much impossible to create memoryleaks when using auto_ptr's. also, they're syntatically identical to standard pointers because of operator overloading. below is an example that illustrates it:

expandcollapse popup

#include <memory>
#include <iostream>
// our data struct. tells us when it exists
struct mystruct {

    int x, y;
    mystruct() {
        std::cout << this << ": constructedn";
    }
    ~mystruct() {
        std::cout << this << ": destructedn";
    }
};
//typedefs so you dont have to write std::auto_ptr all over the place
typedef std::auto_ptr<mystruct> data_ptr;
typedef mystruct * raw_data_ptr;

//returns an auto_ptr
data_ptr data_creater() {

    data_ptr ret(new mystruct);
    ret->x = 3;
    ret->y = 5;
    return ret;

}

//returns a raw pointer
raw_data_ptr data_creater_raw() {

    raw_data_ptr ret(new mystruct);
    ret->x = 3;
    ret->y = 5;
    return ret;

}

//this evil class throws exceptions
class some_class
{
    public: void do_something() { throw 1; }
};

void exampleone() {
    //get data pointer
    raw_data_ptr my_data = data_creater_raw();
    //do some work here
    some_class my_class;
    my_class.do_something();

    //done with work, now we remember (or forget) to free pointer!
    delete my_data;
    // except we of course never reach this point because some_class::do_something() threw an exception.
    // we have now created a memory leak, even though we wrote the code for deletion!
}

void exampletwo() {
    //my_data, a templated auto_ptr, now owns the pointer returned.
    //when auto_ptr goes out of scope, it frees it's pointer.
    data_ptr my_data = data_creater();

    some_class my_class;
    //the simulated exception here is not a problem: c++ ensures destructors
    //are called on exceptions.
    my_class.do_something();

}

data_ptr examplethree() {
    //my_data, a templated auto_ptr, now owns the pointer returned.
    data_ptr my_data = data_creater();
    //we now return the pointer - on copying the auto_ptr loses ownership
    // - and even if it doesn't get captured, it will still be released!
    return my_data;
}

int _tmain(int argc, _TCHAR* argv[])
{
    std::cout << "Example onen";
    try {
        exampleone();
    }
    catch(...) {}

    std::cout << "Example twon";
    try {
        exampletwo();
    }
    catch(...) {}

    std::cout << "Example three:n";
    examplethree();

    system("pause");

    return 0;
}

boost or the new unique_ptr in c++0x11 will however allow to rewrite your example your by substituting char * with unique_ptr<char[]> (more or less) Edited October 13, 2012 by Shaggi

[Andreik]

Ok, thanks for your help. Now I need to look close at your example and learn.