Destructor will erroneously attempt to delete memory that was not allocated?

Hi,

First of all, thanks for reading this.

Chapter 11 in Ivor Horton's Beginning Visual C++ 2008 is dedicated to debugging. You start off typing code exactly as presented and then as the chapter progresses the author leads you through the debugging functionality included with the Visual C++ IDE by fixing the provided code.

As part of this debugging process you take this default constructor for the Name class:

Code:

// Default constructor
Name::Name()
{
    pFirstname = pSurname = "\0";
}

and modify it like so when including a destructor for the class:

Code:

// Default constructor
Name::Name()
{
    pFirstname = new char[1];
    pSurname = new char[1];

    pFirstname[0] = pSurname[0] = '\0'; // Store null character
}

the destructor:

Code:

// Destructor; release memory for the two data members
Name::~Name()
{
    delete[] pFirstname;
    delete[] pSurname;
}

The author's justification for the inclusion of the memory allocation involving the pFirstname and pSurname pointers:

You also should make the default constructor work properly. If the default constructor doesn't allocate memory in the free store, you have the possibility that the destructor will erroneously attempt to delete memory that was not allocated in the free store.

Something about that didn't seem right, so after further research I've found several sources that state that calling delete[] on a null pointer is legal and is a no-op (no effect).

Bottom line: Is the author wrong or am I misunderstanding his statement?

I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?

Originally Posted by Rodaxoleaux

I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?

Calling delete on a null pointer is a guaranteed no-op in C++. If your compiler generates code that causes a null pointer deletion to crash, throw it away and use something else.

Is the author wrong or am I misunderstanding his statement?

I think you misunderstood the author.

Just in case though, using `delete' with a null pointer is fine.

That out of the way, he was probably referring to using `delete' with an uninitialized (thus having an unknown value) pointer.

If you don't allocate anything set the pointer to null and you'll be fine.

I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash;

It doesn't.

At least it does for me with MinGW.

It didn't.

You did something else wrong. You, very likely, forgot to set the pointer to null after freeing the memory from some earlier point.

Soma

Originally Posted by deoren

Something about that didn't seem right, so after further research I've found several sources that state that calling delete[] on a null pointer is legal and is a no-op (no effect).

Bottom line: Is the author wrong or am I misunderstanding his statement?

If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.

Just initializing your pointers to null in the default constructor would be a much superior way.

Also note that the proper way to initialize member variables in a constructor is to use initialization lists:

Code:

Name::Name() : pFirstname( 0 ), pSurname( 0 )
{
}

If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.

I haven't read the book so I'm just speculating, but I imagine that the poster simply misunderstood what the author is doing.

`""\0' and '\0' are very different beasts.

I'm guessing that the intent of the example was to show initialization of an array with the allocation specifically as a means to simplify other components.

*shrug*

Then of course, I can tell you several books that have done perfectly mindless things with pointer so I certainly may be giving the author too much credit.

Soma

Originally Posted by Rodaxoleaux

I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?

Not sure. Here are the sources were I found the statement that it was allowed:

• StackOverflow comment #3194802
• Google Books, C++ Primer Plus, 6e, bottom of page

Originally Posted by phantomotap

If you don't allocate anything set the pointer to null and you'll be fine.

Originally Posted by antred

If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.

That is what I was thinking also. Using that approach resulted in zero issues and no memory leaks reported (that happened to be the focus of the end of the chapter).

Originally Posted by antred

Also note that the proper way to initialize member variables in a constructor is to use initialization lists:

Code:

Name::Name() : pFirstname( 0 ), pSurname( 0 )
{
}

Thanks for that. Thankfully it was covered a few chapters back and I took extensive notes on it. Ironically I forget exactly why it is better, but I remember it had to do with performance reasons (for classes without inheritance) and is the approach used to initialize base class objects.

Originally Posted by phantomotap

I haven't read the book so I'm just speculating, but I imagine that the poster simply misunderstood what the author is doing.

`""\0' and '\0' are very different beasts.

I'm guessing that the intent of the example was to show initialization of an array with the allocation specifically as a means to simplify other components.

Yeah, I don't know. It wasn't very clear as you started off just typing in everything exactly as it was presented, including any errors you came across (was hard not to correct them as they were provided). Then throughout the chapter the author showed you how to correct them using the provided debugging utilities within the IDE.

I think my question has been answered though, and that the 1 byte allocation for the character arrays is mostly pointless since calling delete[] on a null pointer is legal C++ code.

Thanks for all of the feedback.

Originally Posted by deoren

Thanks for that. Thankfully it was covered a few chapters back and I took extensive notes on it. Ironically I forget exactly why it is better, but I remember it had to do with performance reasons (for classes without inheritance) and is the approach used to initialize base class objects.

The difference is that with initialization lists you actually INITIALIZE your members to specific values. Your first approach would first default initialize your members (or not initialize them at all, if they're built-in types such as int, float, pointers, etc.) and then ASSIGN values to them. If your member variables were of types that have a non-trivial default constructor, the work done by their default constructors would essentially be redundant.

Also, if you ever have member variables that are const or members of type reference to xyz, then initialization lists are the ONLY way to initialize those since references can never be reseated and const variables can't be assigned to.

1) Deleting a NULL pointer is safe (a no-op) according to the standards.

2) Any operation on an uninitialised pointer that involves retrieving its value (dereferencing it, deleting it, etc) gives undefined behaviour.

3) Operator delete on a pointer gives undefined behaviour unless that pointer results from the corresponding operator new (i.e. operator new [] has to be matched with operator delete[]).

4) Invoking operator delete more than once on a non-NULL pointer yields undefined behaviour. operator delete does NOT change the value of the pointer.

In this particular case, the initialisation "pFirstname = pSurname = "\0";" means that pFirstName and pSurName have not been created with operator new. So operator delete should NEVER be used on those pointers.

The more general principle here is that constructors (and ALL other public member functions) should be consistent in leaving the object in a valid state. If the destructor uses operator delete on any pointers, then the constructors and ALL public member functions should ensure those pointers are either NULL or the result of the corresponding operator new. If the behaviour of constructors and public member functions does not ensure this, then the destructor will exhibit undefined behaviour.

Personally, in the example Name class given by the OP, I would not even supply a default constructor. I would provide suitable non-default constructors (for example, one that accepts a string), ensure those constructors (and also all public member functions) behave consistently with expectations of the destructor, NEVER create an instance of Name until I have enough information to invoke a non-default constructor, and ensure the object is destroyed (passes out of scope, or - if it is created with operator new - it is deleted) when no longer needed. The technique of "create an object with NULL state and initialise properly later" is error-prone (e.g. the programmer can forget to initialise it later) and unnecessary (an object can be created when needed in C++, and it is unnecessary to create an object before it is needed).

the 1 byte allocation for the character arrays is mostly pointless

O_o

Maybe.

It depends.

Let's analyze the posted methods to view what grumpy posted.

(It doesn't matter what the author intended.)

Code:

pFirstname = pSurname = "\0";

Code:

pFirstname = pSurname = 0;

Code:

pFirstname = new char[1];
pSurname = new char[1];
pFirstname[0] = pSurname[0] = '\0';

(1): You can't safely initialize pointers like this if you are going to be allocating memory later because you can't portably and reliably determine if you should delete the memory without using an otherwise unnecessary flag.

(2): You can safely use `delete' because null is a valid value for "deletion".

(3): As with "2" you can safely use `delete', but this approach may buy you something. You can safely pass null to `delete' but a lot of string and locale functions are not guaranteed to play nicely if passed null. However, a lot of these same functions work perfectly well with string that simply have length zero. That's a situation which can greatly simplify your use of such functions because your constructor has guaranteed that the pointers will not be null. That said, a slightly larger buffer would be more appropriate in practice.

Soma

Originally Posted by antred

The difference is that with initialization lists you actually INITIALIZE your members to specific values. Your first approach would first default initialize your members (or not initialize them at all, if they're built-in types such as int, float, pointers, etc.) and then ASSIGN values to them. If your member variables were of types that have a non-trivial default constructor, the work done by their default constructors would essentially be redundant.

Also, if you ever have member variables that are const or members of type reference to xyz, then initialization lists are the ONLY way to initialize those since references can never be reseated and const variables can't be assigned to.

Well said and thanks for the reminder. I knew there were many reasons given why initialization lists were preferred but I couldn't recall the details. For the most part I've been going chapter by chapter through the book and it's been slow going. Usually once I make it through a chapter I've forgotten many of the details of earlier chapters. I guess it's the price you pay when you don't have a lot of time to

Originally Posted by phantomotap

Code:

pFirstname = new char[1];
pSurname = new char[1];
pFirstname[0] = pSurname[0] = '\0';

(3): As with "2" you can safely use `delete', but this approach may buy you something. You can safely pass null to `delete' but a lot of string and locale functions are not guaranteed to play nicely if passed null. However, a lot of these same functions work perfectly well with string that simply have length zero. That's a situation which can greatly simplify your use of such functions because your constructor has guaranteed that the pointers will not be null. That said, a slightly larger buffer would be more appropriate in practice.

Soma

Interesting, and good to know. I was taking the author literally when he said:

If the default constructor doesn’t allocate memory in the free store, you have the possibility that the destructor will erroneously attempt to delete memory that was not allocated in the free store.

My thoughts were that the pointers could be set to null if the entire goal was to make sure the pointers were valid targets for the delete[] operator.

Code:

// Default constructor
Name::Name()
{
    pFirstname = pSurname = '\0';
}

Your comment about string and locale functions expecting non-null strings is a good reminder to watch out for those situations.

This would also be the perfect time to start learning about The Rule of three (C++)

Originally Posted by phantomotap

You did something else wrong. You, very likely, forgot to set the pointer to null after freeing the memory from some earlier point.

Actually, that sounds about right. Anyway. I learned something.

I wanted to reply back and mention that while looking over the errata for the next chapter I found an errata entry for the author's example code that I posted here:

731 Error in Code
Default constructor (Name::Name) allocates a one-byte array so that the destructor can safely delete[] it.

However this is unnecessary provided the default constructor initialises the array members to 0. This is because the delete[] operator will do nothing if the pointer is null (0).

Thanks again to everyone for their help.