Memory Management

[rodrigo.gloria@gmail.com]

How am I supposed to release memory of a struct I have created:

#include .....

struct MyData {
int size;
char *buffer ;
};

MyData *allocate_mydat a(char *str)
{
MyData *newData;
newData->size = strlen(str);
memcpy(newData->buffer,str,new Data->size);
return newData;
}

void release_mydata( MyData *d)
{
if ( d && d->buffer ) delete d->buffer;
if ( d ) delete d;
}

void main()
{
MyData *d;
char *text = "Hello World";

d = allocate_mydata (text);

release_mydata( d);

}

[Jakob Bieling]

Re: Memory Management

rodrigo.gloria@ gmail.com wrote:[color=blue]
> How am I supposed to release memory of a struct I have created:
>
> #include .....
>
> struct MyData {
> int size;
> char *buffer ;
> };
>
> MyData *allocate_mydat a(char *str)
> {
> MyData *newData;[/color]

This creates a *pointer* to MyData .. which points to nowhere ..
[color=blue]
> newData->size = strlen(str);[/color]

By using operator-> you access the memory pointed to by newData ..
you access memory that is nowhere!
[color=blue]
> memcpy(newData->buffer,str,new Data->size);[/color]

And again.
[color=blue]
> return newData;
> }
>
> void release_mydata( MyData *d)
> {
> if ( d && d->buffer ) delete d->buffer;[/color]

Every call to delete must have a matching new and every call to
delete[] must have a matching new[]. I do not see any 'new' in your
code.
[color=blue]
> if ( d ) delete d;[/color]

And again.
[color=blue]
> }
>
> void main()[/color]

main *always* returns int. If someone says otherwise, they are
lying.
[color=blue]
> {
> MyData *d;
> char *text = "Hello World";
>
> d = allocate_mydata (text);
>
> release_mydata( d);
>
> }[/color]

I hope my remarks gave you some clues on what you are doing wrong.
If not, you should read up on dynamic memory allocation using new and
new[]. Better yet, read up on "std::strin g" and strategies to avoid
dynamic memory allocations where possible.

hth
--
jb

(reply address in rot13, unscramble first)

[Marcus Kwok]

Re: Memory Management

rodrigo.gloria@ gmail.com wrote:[color=blue]
> How am I supposed to release memory of a struct I have created:
>
> #include .....
>
> struct MyData {
> int size;
> char *buffer ;
> };
>
> MyData *allocate_mydat a(char *str)[/color]

I think it should be

MyData* allocate_mydata (const char* str)

which will allow you to pass in string literals as well, and also
documents that you do not intend to change str.
[color=blue]
> {
> MyData *newData;[/color]

You never actually allocate any memory for newData.

MyData* newData = new MyData;
[color=blue]
> newData->size = strlen(str);
> memcpy(newData->buffer,str,new Data->size);[/color]

You also never allocate any memory for buffer.

strcpy() may be better than memcpy since you don't have to pass the
size.

newData->size = strlen(str);
newData->buffer = new char[newData->size + 1]; // Add 1 for the null terminator
std::strcpy(new Data->buffer, str);
[color=blue]
> return newData;
> }
>
> void release_mydata( MyData *d)
> {
> if ( d && d->buffer ) delete d->buffer;[/color]

Deleting a 0-pointer is OK, so you can just have

if (d) delete[] d->buffer;

but since d->buffer is an array of characters allocated with new[], you
must use delete[] (instead of plain delete).
[color=blue]
> if ( d ) delete d;[/color]

Same thing here, just do

delete d;

and don't worry about checking "if (d)" first.
[color=blue]
> }
>
> void main()[/color]

main() ALWAYS returns an int.

int main()
[color=blue]
> {
> MyData *d;
> char *text = "Hello World";
>
> d = allocate_mydata (text);
>
> release_mydata( d);
>
> }[/color]

However, depending on what you are doing, it may be better to use
std::string, which will handle all memory allocation/deallocation for
you:

#include <string>

struct MyData {
int size; // may be unnecessary since std::string tracks its own size
std::string buffer;
};

--
Marcus Kwok

[rodrigo.gloria@gmail.com]

Re: Memory Management

MyData *allocate_mydat a(char *str)
{
MyData *newData;
newData->size = strlen(str);
newData->buffer = new char[size];
memcpy(newData->buffer,str,new Data->size);
return newData;
}

void release_mydata( MyData *d)
{
if ( d && d->buffer ) delete [] d->buffer;
if ( d ) delete d;
}

int main()
{
MyData *d;
char *text = "Hello World";

d = new MyData;

d = allocate_mydata (text);
release_mydata( d);

return 0;
}

[Jim Langston]

Re: Memory Management

<rodrigo.gloria @gmail.com> wrote in message
news:1142632118 .444152.27910@j 52g2000cwj.goog legroups.com...[color=blue]
> MyData *allocate_mydat a(char *str)
> {
> MyData *newData;
> newData->size = strlen(str);
> newData->buffer = new char[size];
> memcpy(newData->buffer,str,new Data->size);
> return newData;
> }
>
> void release_mydata( MyData *d)
> {
> if ( d && d->buffer ) delete [] d->buffer;
> if ( d ) delete d;
> }
>
> int main()
> {
> MyData *d;
> char *text = "Hello World";
>
> d = new MyData;
>
> d = allocate_mydata (text);
> release_mydata( d);
>
> return 0;
> }[/color]

Why don't you just have MyData's destructor delete the memory and in that
case allocate it to? Your MyData class seems to be the one that "owns" the
memory, so it should be responsible for allocating it and deleting it. In
which case allocate_mydata could be moved into MyData itself.

void MyData::SetStri ng(const char* str)
{
delete[] buffer;
buffer = new char[strlen(str) + 1];
memcpy( newData->buffer, str, strlen(str) );
}

And in MyData's destructor:

MyData::~MyData ()
{
delete[] buffer;
}

and in MyData's constructor:

MyData::MyData( ) : buffer( NULL ) {...}

use 0 if you want instead of NULL.

[Jim Langston]

Re: Memory Management


"Jim Langston" <tazmaster@rock etmail.com> wrote in message
news:%ZISf.299$ AR1.51@fe02.lga ...[color=blue]
> <rodrigo.gloria @gmail.com> wrote in message
> news:1142632118 .444152.27910@j 52g2000cwj.goog legroups.com...[color=green]
>> MyData *allocate_mydat a(char *str)
>> {
>> MyData *newData;
>> newData->size = strlen(str);
>> newData->buffer = new char[size];
>> memcpy(newData->buffer,str,new Data->size);
>> return newData;
>> }
>>
>> void release_mydata( MyData *d)
>> {
>> if ( d && d->buffer ) delete [] d->buffer;
>> if ( d ) delete d;
>> }
>>
>> int main()
>> {
>> MyData *d;
>> char *text = "Hello World";
>>
>> d = new MyData;
>>
>> d = allocate_mydata (text);
>> release_mydata( d);
>>
>> return 0;
>> }[/color]
>
> Why don't you just have MyData's destructor delete the memory and in that
> case allocate it to? Your MyData class seems to be the one that "owns"
> the memory, so it should be responsible for allocating it and deleting it.
> In which case allocate_mydata could be moved into MyData itself.
>
> void MyData::SetStri ng(const char* str)
> {
> delete[] buffer;
> buffer = new char[strlen(str) + 1];
> memcpy( newData->buffer, str, strlen(str) );
> }
>
> And in MyData's destructor:
>
> MyData::~MyData ()
> {
> delete[] buffer;
> }
>
> and in MyData's constructor:
>
> MyData::MyData( ) : buffer( NULL ) {...}
>
> use 0 if you want instead of NULL.[/color]

Incidently, you need a custom copy constructor and assignment operator also
to allocate the memory for the object.

[benben]

Re: Memory Management

rodrigo.gloria@ gmail.com wrote:[color=blue]
> How am I supposed to release memory of a struct I have created:
>
> #include ....
>
> struct MyData {
> int size;
> char *buffer ;
> };
>
> MyData *allocate_mydat a(char *str)
> {
> MyData *newData;
> newData->size = strlen(str);
> memcpy(newData->buffer,str,new Data->size);
> return newData;[/color]

should be:

std::auto_ptr<M yData> newData(new MyData);

newData->size = int(strlen(str) );

newData->buffer = new char[newData->size + 1];
strcpy(newData->buffer, str);

return newData.release ();
[color=blue]
> }
>
> void release_mydata( MyData *d)
> {
> if ( d && d->buffer ) delete d->buffer;
> if ( d ) delete d;[/color]

should be:

if (d != 0)
if (d->buffer != 0)
delete[] d->buffer;

delete d;

[color=blue]
> }
>
> void main()
> {
> MyData *d;
> char *text = "Hello World";
>
> d = allocate_mydata (text);
>
> release_mydata( d);
>
> }
>[/color]

Of course, if you can actually change MyData it is much better just to
use std::string to replace size and buffer:

class MyData
{
std::string buff;

public:
explicit MyData(const char* s):buff(s){}
};


You code can then be simplified as:

MyData* d = new MyData("Hello, world!");
delete d;

Or (if you don't need to do dynamic allocation, then simply) just one line:

MyData d("Hello world!");

Deallocation happens automatically when d runs out of scope.

Regards,
Ben

[Jakob Bieling]

Re: Memory Management

rodrigo.gloria@ gmail.com wrote:
[color=blue]
> MyData *allocate_mydat a(char *str)
> {
> MyData *newData;[/color]

Here you still have a problem. 'newData' points to nowhere.
[color=blue]
> newData->size = strlen(str);
> newData->buffer = new char[size];
> memcpy(newData->buffer,str,new Data->size);
> return newData;
> }
>
> void release_mydata( MyData *d)
> {
> if ( d && d->buffer ) delete [] d->buffer;
> if ( d ) delete d;
> }
>
> int main()
> {
> MyData *d;
> char *text = "Hello World";
>
> d = new MyData;[/color]

Okay, you allocate a new MyData structure and assign a pointer to
that memory to 'd'.
[color=blue]
> d = allocate_mydata (text);[/color]

But here you overwrite that pointer with the return value from
'allocate_mydat a'. This means, the pointer to the memory you just
allocated will be lost and you will never be able to free that memory
(ie. you have a memory leak).

The solution is to move the allocation into 'allocate_mydat a' ..
right there where I told you that you still have a problem :)
[color=blue]
> release_mydata( d);
>
> return 0;
> }[/color]

Once the errors I pointed out are fixed, you have a semantically
correct program.

You should still consider reading more about dynamic memory
(especially about the question 'when is it appropriate to use dynamic
memory?'), C-style strings as well as learning more about the Standard
Template Library and the containers it provides.

hth
--
jb

(reply address in rot13, unscramble first)

[benben]

Re: Memory Management

> Once the errors I pointed out are fixed, you have a semantically[color=blue]
> correct program.
>[/color]

Once the OP clears the errors you kindly pointed out he still has
potential memory leaks.

Regards,
Ben

[benben]

Re: Memory Management

[...][color=blue][color=green]
>>
>> void release_mydata( MyData *d)
>> {
>> if ( d && d->buffer ) delete d->buffer;[/color]
>
> Deleting a 0-pointer is OK, so you can just have
>
> if (d) delete[] d->buffer;
>
> but since d->buffer is an array of characters allocated with new[], you
> must use delete[] (instead of plain delete).[/color]

If d == 0 then the statement

delete[] d->buffer;

has undefined behavior.

[...]

Regards,
Ben

[Jakob Bieling]

Re: Memory Management

benben <benhongh@yahoo .com.au> wrote:[color=blue][color=green]
>> Once the errors I pointed out are fixed, you have a semantically
>> correct program.[/color][/color]
[color=blue]
> Once the OP clears the errors you kindly pointed out he still has
> potential memory leaks.[/color]


Exception safety is a different chapter. This is why I suggested the
OP read more about dynamic memory allocation, which will cover this
issue sooner or later.

regards
--
jb

(reply address in rot13, unscramble first)

[Marcus Kwok]

Re: Memory Management

benben, please do not snip the attributions, so people know who wrote
what. I have added mine back in.

Marcus Kwok wrote:[color=blue][color=green][color=darkred]
>>> void release_mydata( MyData *d)
>>> {
>>> if ( d && d->buffer ) delete d->buffer;[/color]
>>
>> Deleting a 0-pointer is OK, so you can just have
>>
>> if (d) delete[] d->buffer;
>>
>> but since d->buffer is an array of characters allocated with new[], you
>> must use delete[] (instead of plain delete).[/color][/color]

benben <benhongh@yahoo .com.au> wrote:[color=blue]
> If d == 0 then the statement
>
> delete[] d->buffer;
>
> has undefined behavior.[/color]

Right, which is why I check it first. In the statement

if (d) delete[] d->buffer;

If d == 0 then the delete[] statement is not executed.


To simplify, the check

if (d) delete d;

is redundant, and can be replaced with just

delete d;


Similarly, the statement

if (d && d->buffer) delete[] d->buffer;

can be replaced with just

if (d) delete[] d->buffer;

--
Marcus Kwok