Does this constitute a memory leak?

N

Nikola

#include <stdio.h>
#include <string.h>

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * %s.\n", b);
a = new char[strlen(b)];
strcpy(a, b);
}

tst(const tst &copy) {
printf("Copy constructor for %s.\n", copy.a);
a = new char[strlen(copy.a)];
strcpy(a, copy.a);
}

~tst() {
printf("Destructor for %s.\n", this->a);
delete [] a;
}
};

tst &func3() {
printf("FUNC3\n");
tst *a = new tst("3456");
return *a; // <---- THIS (Copy constructor is called)
}

Basically, I want func3() to return either a reference or value of (but
not a pointer to) an object of type tst, but I'm not sure if this
solution results in a memory leak or not. Also, func3 must have no
output parameters.

A weird thing that happens is that in this test example, a copy
constructor for a is called. But in the real problem which made me
create this test, the copy constructor is NOT called and I don't see the
difference - I have the same instantiation (through a constructor which
takes const char * as a parameter) and the same return-by-reference case
there.

If it will make any sense, here's the original function:

String &DocumentBuilder::GetSubstring(const char *WholeDoc, int CPos,
int Len) {
printf("Getting substring beginning at %d of length %d.\n", CPos, Len);
char *resStr = new char[Len + 1];
resStr = (char *)memset(resStr, '0', Len);
resStr[Len] = 0;
String *inResult = new String(resStr);
String &Result = *inResult;
delete [] resStr;
for (int x = CPos; x < CPos + Len; ++x)
Result.SetChar(x - CPos, WholeDoc[x]);
Result.SetChar(Len, 0);
printf("After getting substring, value is %s.\n", (char *)Result);
return *inResult; // Copy constructor is NOT called
}

Basically, this function gets a substring from WholeDoc array which in
general contains binary data with some ASCII character sequences. String
represents a string, a class I've created to simplify their
manipulation. Constructors in String class are the same as the ones in
tst class I wrote. String::SetChar(int x, char a) sets character on
position x to value a. Operator (char *) converts an object of type
String to a character array containing characters that make up the string.

DocumentBuilder is a class that converts binary data contained in the
array passed to GetSubstring to something more manageable in further
processing. GetSubstring is a private static function in that class. I
don't think this should play much of a role here.

I'd be thankful if someone can explain what's going on in each example also.

Nikola
 
K

Kai-Uwe Bux

Nikola said:
#include <stdio.h>
#include <string.h>

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * %s.\n", b);
a = new char[strlen(b)];
strcpy(a, b);

I think, you are 1 byte short here
}

tst(const tst &copy) {
printf("Copy constructor for %s.\n", copy.a);
a = new char[strlen(copy.a)];
strcpy(a, copy.a);

and here.
}

~tst() {
printf("Destructor for %s.\n", this->a);
delete [] a;
}
};

tst &func3() {
printf("FUNC3\n");
tst *a = new tst("3456");
return *a; // <---- THIS (Copy constructor is called)
}

Basically, I want func3() to return either a reference or value of (but
not a pointer to) an object of type tst, but I'm not sure if this
solution results in a memory leak or not.

It is, unless the client deletes the memory. (Each and every new has to be
matches with one and only one delete). One way of doing that would be

tst & get = func3();
...
delete ( &get );

As you can see, you gain nothing over pointers (except that programmers are
more likely to forget the delete).
Also, func3 must have no output parameters.
Why?

A weird thing that happens is that in this test example,

There is no test example.
a copy constructor for a is called.

When I supply

int main ( void ) {
tst & get = func3();
delete ( &get );
}

I get:

FUNC3
Construct from char * 3456.
Destructor for 3456.

So it appears that the copy constructor is not called.


Could it be that you do:

int main ( void ) {
tst get = func3();
// delete ( &get );
}

In that case, the variable get is copy-initialized from the returned tst&.
(But you leak memory.)


[snip]


Best

Kai-Uwe Bux
 
N

Nikola

Kai-Uwe Bux said:
Nikola said:
#include <stdio.h>
#include <string.h>

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * %s.\n", b);
a = new char[strlen(b)];
strcpy(a, b);

I think, you are 1 byte short here
}

tst(const tst &copy) {
printf("Copy constructor for %s.\n", copy.a);
a = new char[strlen(copy.a)];
strcpy(a, copy.a);

and here.

Ah yes, thanks. I write bad test classes. :)
}

~tst() {
printf("Destructor for %s.\n", this->a);
delete [] a;
}
};

tst &func3() {
printf("FUNC3\n");
tst *a = new tst("3456");
return *a; // <---- THIS (Copy constructor is called)
}

Basically, I want func3() to return either a reference or value of (but
not a pointer to) an object of type tst, but I'm not sure if this
solution results in a memory leak or not.

It is, unless the client deletes the memory. (Each and every new has to be
matches with one and only one delete). One way of doing that would be

tst & get = func3();
...
delete ( &get );

As you can see, you gain nothing over pointers (except that programmers are
more likely to forget the delete).
Also, func3 must have no output parameters.

Why?

I made that restriction here because a lot of the code is already
written and functions don't have output parameters. If I were to change
that I'd have to rewrite much of the code and I kind of want to do that
only as my last resort. Since I know how to do that anyway, I wanted to
also spare you the time and effort of constructing such functions as
it's unnecessary.
There is no test example.

Yes indeed. Forgot to c/p. Sorry.
When I supply

int main ( void ) {
tst & get = func3();
delete ( &get );
}

I get:

FUNC3
Construct from char * 3456.
Destructor for 3456.

So it appears that the copy constructor is not called.


Could it be that you do:

int main ( void ) {
tst get = func3();
// delete ( &get );
}

In that case, the variable get is copy-initialized from the returned tst&.
(But you leak memory.)

Yes, that's what I do. I'll switch to what you do.
[snip]


Best

Kai-Uwe Bux

Thanks,
Nikola Novak
 
N

Nikola

Hmm, I think I've found where my test example differs from the actual
program. Here's the fixed test program, now with examples:


#include <stdio.h>
#include <string.h>

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * %s.\n", b);
a = new char[strlen(b) + 1];
strcpy(a, b);
}

tst(const tst &copy) {
printf("Copy constructor for %s.\n", copy.a);
a = new char[strlen(copy.a) + 1];
strcpy(a, copy.a);
}

~tst() {
printf("Destructor for %s.\n", this->a);
delete [] a;
}
};

tst &func3() {
printf("FUNC3\n");
tst *a = new tst("3456");
return *a; // <---- THIS (Copy constructor is called)
}

int main(void) {
tst a = func3(); // this is how I called - copy constructor is called
tst &b = func3(); // Kai-Uwe called this - no copy constructor calls
tst c = ""; // the way it's done in my program
c = func3(); // no copy constructor is called

delete (&b);
// How do I free c?
return 0;
}

This is the output:

FUNC3
Construct from char * 3456.
Copy constructor for 3456.
FUNC3
Construct from char * 3456.
Construct from char * .
FUNC3
Construct from char * 3456.
Destructor for 3456.
Destructor for 3456.
Destructor for 3456.

I suppose the way I do it is rather bad because first memory is taken
for tst c = ""; (it's member a, to be more precise) and it's never freed
and then it's modified to point to another string, thus forever losing
the pointer to what was previously allocated.

In any case, I think I'm in for some modifications.

Nikola Novak
 
K

Kai-Uwe Bux

Nikola said:
Hmm, I think I've found where my test example differs from the actual
program. Here's the fixed test program, now with examples:


#include <stdio.h>
#include <string.h>

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * %s.\n", b);
a = new char[strlen(b) + 1];
strcpy(a, b);
}

tst(const tst &copy) {
printf("Copy constructor for %s.\n", copy.a);
a = new char[strlen(copy.a) + 1];
strcpy(a, copy.a);
}

You will want to add an assignment operator:

tst & operator= ( tst lhs ) {
std::swap( a, lhs.a );
return ( *this );
}
~tst() {
printf("Destructor for %s.\n", this->a);
delete [] a;
}
};

tst &func3() {
printf("FUNC3\n");
tst *a = new tst("3456");
return *a; // <---- THIS (Copy constructor is called)
}

You might consider returning by value:

tst func4 ( void ) {
return ( tst( "3456" ) );
}
int main(void) {
tst a = func3(); // this is how I called - copy constructor is called
tst &b = func3(); // Kai-Uwe called this - no copy constructor calls
tst c = ""; // the way it's done in my program
c = func3(); // no copy constructor is called

Here, you _use_ the assignment operator. I am positive that the compiler
generated one is _not_ what you want.
delete (&b);
// How do I free c?

Freeing c is not the problem (the destructor will do that). The problem lies
in the the pointer allocated by func3 from whose pointee c is initialized.
return 0;
}

This is the output:

FUNC3
Construct from char * 3456.
Copy constructor for 3456.
FUNC3
Construct from char * 3456.
Construct from char * .
FUNC3
Construct from char * 3456.
Destructor for 3456.
Destructor for 3456.
Destructor for 3456.

I suppose the way I do it is rather bad because first memory is taken
for tst c = ""; (it's member a, to be more precise) and it's never freed
and then it's modified to point to another string, thus forever losing
the pointer to what was previously allocated.

That problem will be solved by an assignment operator. In that case

tst c = "";
c = tst( "3456" );

would be fine and the destructors of all temporaries involved will get rid
of all memory allocated along the way.
In any case, I think I'm in for some modifications.

The typical C++ way is to return by value.


Best

Kai-Uwe Bux
 
T

Thomas J. Gritzan

Nikola said:
Hmm, I think I've found where my test example differs from the actual
program. Here's the fixed test program, now with examples: [...]
int main(void) {
tst a = func3(); // this is how I called - copy constructor is called
tst &b = func3(); // Kai-Uwe called this - no copy constructor calls
tst c = ""; // the way it's done in my program
c = func3(); // no copy constructor is called [...]
I suppose the way I do it is rather bad because first memory is taken
for tst c = ""; (it's member a, to be more precise) and it's never freed
and then it's modified to point to another string, thus forever losing
the pointer to what was previously allocated.

When you do
c = func3();
you call the copy assignment operator, but you didn't define it, so the
compiler supplied copy assignment operator is used.

The copy assignment operator usually looks like this:

tst& operator=(const tst& rhs)
{
tst temp(rhs); // copy and...
std::swap(a, temp.a); // ...swap
return *this;
}

While your string class has to use new[] internally, the client code
shouldn't use new at all.

tst func3()
{
tst a("3456");
return a; // <---- Copy constructor is called
}

Since there's no new here, you don't need delete. No chance to leak memory.
 
N

Nikola

Thomas said:
When you do
c = func3();
you call the copy assignment operator, but you didn't define it, so the
compiler supplied copy assignment operator is used.

The copy assignment operator usually looks like this:

tst& operator=(const tst& rhs)
{
tst temp(rhs); // copy and...
std::swap(a, temp.a); // ...swap
return *this;
}

While your string class has to use new[] internally, the client code
shouldn't use new at all.

tst func3()
{
tst a("3456");
return a; // <---- Copy constructor is called
}

Actually, it isn't. In the following example:

#include <stdio.h>
#include <string.h>
#include <algorithm>

using namespace std;

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * \"%s\".\n", b);
a = new char[strlen(b)];
strcpy(a, b);
}

tst(const tst &copy) {
printf("Copy constructor for \"%s\".\n", copy.a);
a = new char[strlen(copy.a)];
strcpy(a, copy.a);
}

~tst() {
printf("Destructor for \"%s\".\n", this->a);
delete [] a;
a = NULL;
}

tst &operator=(const char *Value) {
printf("String = const char *: ");
printf("Replacing \"%s\" with \"%s\".\n", a, Value);
if (a != NULL) delete [] a;
a = new char[strlen(Value) + 1];
strcpy(a, Value);
return *this;
}

tst &operator=(tst Value) {
printf("String = String: ");
if (a != NULL) printf("Replacing \"%s\" with \"%s\".\n", a, Value.a);
else printf("Assigning \"%s\".\n", Value.a);
tst temp(Value);
std::swap(a, temp.a);
return *this;
}
};

tst func1() {
printf("FUNC1\n");
tst a("1234");
return a;
}

int main(void) {
{
tst d = func1();
printf("--\n");
tst e = "";
printf("--\n");
e = func1();
printf("--\n");
}
printf("Done\n");
return 0;
}

This is the output:

FUNC1
Construct from char * "1234".
--
Construct from char * "".
--
FUNC1
Construct from char * "1234".
String = String: Replacing "" with "1234".
Copy constructor for "1234".
Assignment complete.
Destructor for "".
Destructor for "1234".
--
Destructor for "1234".
Destructor for "1234".
Done

As you can see, the copy constructor is not called when returning from
func1().
> Since there's no new here, you don't need delete. No chance to leak
memory.

Yes, I had it solved like this before, but without the assignment
operator I got an error saying I freed the same memory twice or some
such thing.

Thanks,
Nikola
 
N

Nikola

Kai-Uwe Bux wrote:
That problem will be solved by an assignment operator. In that case

tst c = "";
c = tst( "3456" );

would be fine and the destructors of all temporaries involved will get rid
of all memory allocated along the way.


The typical C++ way is to return by value.

Thanks, the assignment operator solved the problem. You can also check
my reply to Thomas J. Gritzan.

Nikola
 
T

Thomas J. Gritzan

Nikola said:
Thomas said:
When you do
c = func3();
you call the copy assignment operator, but you didn't define it, so the
compiler supplied copy assignment operator is used.

The copy assignment operator usually looks like this:

tst& operator=(const tst& rhs)
{
tst temp(rhs); // copy and...
std::swap(a, temp.a); // ...swap
return *this;
}

While your string class has to use new[] internally, the client code
shouldn't use new at all.

tst func3()
{
tst a("3456");
return a; // <---- Copy constructor is called
}

Actually, it isn't. In the following example:

#include <stdio.h>
#include <string.h>
#include <algorithm>

using namespace std;

class tst {
public:
char *a;

tst() {
printf("Default constructor.\n");
a = new char[1];
a[0] = 0;
}

tst(const char *b) {
printf("Construct from char * \"%s\".\n", b);
a = new char[strlen(b)];
strcpy(a, b);

You don't allocate enough memory. Buffer overflow when strcpy copies the
null terminator.
}

tst(const tst &copy) {
printf("Copy constructor for \"%s\".\n", copy.a);
a = new char[strlen(copy.a)];
strcpy(a, copy.a);

As above.
}

~tst() {
printf("Destructor for \"%s\".\n", this->a);
delete [] a;
a = NULL;

You don't need to assign NULL. It's the destructor. The member variable
'a' won't exist after this.
}

tst &operator=(const char *Value) {
printf("String = const char *: ");
printf("Replacing \"%s\" with \"%s\".\n", a, Value);
if (a != NULL) delete [] a;

You don't need to check for NULL, since delete[] also does it.
a = new char[strlen(Value) + 1];
strcpy(a, Value);
return *this;
}

You can use the copy & swap idiom for this function, too:

tst temp(Value);
std::swap(a, temp.a);
return *this;
}
tst &operator=(tst Value) {

Here you pass by value. A copy is made from the assigned value.
printf("String = String: ");
if (a != NULL) printf("Replacing \"%s\" with \"%s\".\n", a, Value.a);
else printf("Assigning \"%s\".\n", Value.a);
tst temp(Value);

Here you copy again. Makes two copies. One of the copies would be enough.
When you consult the answer of Kai-Uwe, he wrote this function:

tst & operator= ( tst lhs ) {
std::swap( a, lhs.a );
return ( *this );
}

It makes a copy in the parameter list, but only a swap in the function
body. My version (at the start of this posting) uses pass by reference
and makes the copy explicit in the function body.
Both versions are equivalent, so you can use either, but please avoid
the double copy.
std::swap(a, temp.a);
return *this;
}
};

tst func1() {
printf("FUNC1\n");
tst a("1234");
return a;
}

int main(void) {
{
tst d = func1();
printf("--\n");
tst e = "";
printf("--\n");
e = func1();
printf("--\n");
}
printf("Done\n");
return 0;
}

This is the output:

FUNC1
Construct from char * "1234".
-- [...]
As you can see, the copy constructor is not called when returning from
func1().

The compiler is allowed to optimize away copy constructor calls.
It usually will when it applies the so called NRVO (named return value
optimization).

I assume this class is for learning purposes. You should use std::string
for storing strings, unless you have a good reason not to.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,482
Members
44,901
Latest member
Noble71S45

Latest Threads

Top