Jump to content
Sign in to follow this  
Andreik

Memory release?

Recommended Posts

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!


When the words fail... music speaks

Share this post


Link to post
Share on other sites
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' :)

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Mat

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

  • Like 1

Share this post


Link to post
Share on other sites
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;
}

When the words fail... music speaks

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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. ^_^

#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 by Andreik

When the words fail... music speaks

Share this post


Link to post
Share on other sites
Andreik

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


When the words fail... music speaks

Share this post


Link to post
Share on other sites
Andreik

Oups! It's night here.Thanks. muttley


When the words fail... music speaks

Share this post


Link to post
Share on other sites
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 by Andreik

When the words fail... music speaks

Share this post


Link to post
Share on other sites
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.

Ever wanted to call functions in another process? ProcessCall UDFConsole stuff: Console UDFC Preprocessor for AutoIt OMG

Share this post


Link to post
Share on other sites
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?


When the words fail... music speaks

Share this post


Link to post
Share on other sites
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:

#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 by Shaggi
  • Like 1

Ever wanted to call functions in another process? ProcessCall UDFConsole stuff: Console UDFC Preprocessor for AutoIt OMG

Share this post


Link to post
Share on other sites
Andreik

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


When the words fail... music speaks

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×