Using classes you have to watch for some pitfalls.
So we are going to look at the design of a class for holding a name value pair.

Both name and value are strings (now we should use std::string) but that is too easy so we will be using C strings. Also by using C type strings I will be able to point out some generic resource management things that will help:

Design of the class.
The name part is constant and will never change.
The value part is not constant and may change.


So the intial design looks like this:

I hope there are no errors in the code above.
But even I make mistakes.

The question for you is:
1) What is the dangerous deliberate mistake? i.e. Why would this code not work as expected under normal usage and leave the object in an undefined (and dangerous) state in the real world?
2) What is the side affect delibrate mistake? i.e. Why would this code not work as expected and under normal usage and leave the object in an unexpected state.
3) What would stop this code from working in exceptional circumstances. There is one line that potentially leaves the code in an undefined state. Can you spot it?


Answers by e-mail please.
Public credit for those who spot the problems. More credit if you spot something I missed.

and a promise of no abuse for silly answers :-)

Well I did spot this:

Sorry Idiot mistake.

Problem 1:
A dangerous mistake.

When you manage pointers in an object. You need to make sure define your own versions of 'Default Constructor', 'Copy Constructor' and 'Assignment Operator'.

Remember: 'Managing a pointer' is different to just having a pointer. Managing a pointer means you take ownership of the pointer and take responcability for creating and destroying it.

In the example above I did not defined the 'Copy Constructor'. Therefore the compiler will generate the 'Copy Constructor' if required. The default copy constructor is simple and just does a 'Copy Construction' of each member variable.

Now if we look at some dangerous code that looks inocent:

Problem 2:

In this class we have a one parameter constructor.
It is hidden by the fact that the only constructor takes 2 parameters. But the second parameter has a default value, so in affect we have 2 Constructors.

So whats the problem with one argument constructors?
Well the compiler tries to be smart and can will automatically use 1 parameter default constructor to convert one type into another. How does this affect this example?

Will try this:

The class ValuePair has an assignment operator that takes another ValuePair, but no assignment operator that takes a 'const char*'. So you would think the compiler would complain. But no. It sees that it can convert a 'const char*' into a ValuePair via a one parameter constructor.

So the above code is equivelent too:

After this assignment pair.name = "Value", pair.value="" The value field was overwritten in the assignment operator.

So how do we protect against this?
Any constructor that takes 1 parameter should be prefixed with explicit. This will prevent this side affect from happening. You can then remove the 'explicit' if you think this side affect is desirable (like std::string), but you will usually find that this kind of automatic casting is not what you want.

Problem 3:

When you are writting code in an environment with exceptions. You should write your code so that it can cope with exceptions.

Basically there are types of garantees that your method should try and conform too:
1) A method should leave the object in a valid state.
2) A method should leave the object unchanged.
3) A method should never throw an exception.

Garantee (3) is the best. But if you call other methods or functions this becomes imposable to garantee in C++.
Garantee (2) Is the next best. It indicates that either the method will work completely or it will fail. Nothing in between.
Garantee (1) Is the basic level. And you should try and at least provide this.

So how do I fail above?
In the method setValue() I call new (which is allowed to throw bad_alloc).
What happens if it does?

Will in this case the member 'value' has already been deleted, but 'value' still points at the old memory (remember the new never completed and so no assignment is made to 'value').

Scenario 1:
The stack unwinds and the object will be deleted and the destructor called on memory that is already deleted.

Scenario 2:
You catch the exception before the object is deleted.
Now you have an object that contains an invalid pointer. This can lead to a whole host of problems that are hard to find.


So how do we fix it?

The simpist way is not to delete the memory until you know the new memory has benn allocated:

Problem 4:

If you write a 'Assignment Operator' then you need to think about self assignment. This becomes even more important when you are managing pointers.

Everbody got the above three problems.
But nobody thought of this:

What happens if:

Again in this example the problem is down in setValue().
Left as an excercise if you can spot why.

The solution:

You need to test for self assignment.

I don't think that the operator= works just as one would expect. I personally think a op= should always either work similar to the Copyconstructor or be made private. In this case it is simple to make the op= exception safe using the copy and swap idom. Writting an exception safe swap function for this class is trivial.

Furthermore I personally consider a self assignment to be highly unlikly and therefore, even if it doesn't cost a lot, I prefer not having to compare against this. A self assignment is also a situation one often forgets to consider and gets away with because it's so rare therefore this is a second argument to look for another solution.

The copy and swap idom gets self assinments right without having to consider this special case. This means there is no overheat of the if clause and you always get it right.

Rewritting the setValue function as following would also solve the self assignment problem.

I don't know if you got my email but I suggested using swap to implement op= (if I remember well) so it would have been a no problem.

PS: You forgot the = after operator in the last example.

I did't use the copy/swap technique because I personally don't like it.
But others do and their are arguments for and against.

Why don't you write a new article on the principles and explain to the others what you are taking about as I am sure it went right over most people's heads.

When it comes to Problem 3, what i usually do is delete, then set to 0, then new....anything big against that that i should know about?

lol I am feeling a bit dwarfed after reading dr Voodoos tips!

The trouble with doing:

If the new throws. Is your object in a valid state with x being a NULL pointer?
If you object copes with that situation then fine. But not all code is written that way, for example in the code above it assumes that both name and value are non NULL no cheks are made to see of they are NULL before doing a strlen() so this technique will not work.

Null is a valid state for me usually.
Of course this method is more what i would do for things like images or meshes: If i do the above, yes i would be checking for a NULL object before[edit]er, i mean, at the start of[/edit] procedures, though the objects being much larger than a little string is most likely a check in place each using command wouldnt be so bad.

In the above code.

I have made sure it is effecient by not needing to check for NULL. As I garantee that name and value will never have be NULL and always point at a valid C string.

Of course there is a cost.

1) In this case I will have both strings resident in memory at the point just before the original is deleted.
2) The code is slightly more complex and I need to add a comment to make sure any junior programmer that comes along latter and wants to refactor the code make's sure the envirient holds.

Part 1: Parameter constructor:

since the requirements of the class are that neither name_ nor value_
can be 0 (null) our constructor cannot be exception safe (it must throw)
simply because our operator new can fail allocating

this is the best I could come up with :( please post comments

I do not agree. If a constructor can not construct the object we request then it fails and thus should throw. This would also be true if one of the pointers could be NULL. Null does not reflect the state that the user wants and therefore is an error.

Success report messages like

are likely to get a price for the most funny message but they surly do not reflect what the user wants.

Don't use exception specificatons. For an argumentation on why read what Herb Sutter says. http://www.gotw.ca/publications/mill22.htm

You use way too many try catch blocks. Generally speaking you do not need any try catch block except if you want to catch an exception. You extremly rarly need to rethrow an exception. If one object manages one resource then the destructors will automatically make the program exception safe.

For example in this case you should not manage the string resurces manually but use a string class.

You do know that strlen can't throw?

What is this macro good for?

There is also no reason why one should delgate the construction of the object to a methode instead of doing it in the constructor. At least in this case there isn't.

your answers show that you do not understand what I wrote.

1. that is exactly what I said but then why don't U agree ?

2. Herb Sutters gotw doesn't apply here since we agreed that the constructor cannot be made not to throw so I make it throw what I want.

3. in the first post it was posted that we intentionally don't want to use strings.

4. try using: strlen((char *) 0x1);
couldn't find anyting in the standard where it sais that strlen cannot throw so I just used it to test the pointer validity (*)

5. that macro is good for (comment tells it)
initMember_(name_, name, "No Name", sizeof("NoName"));
(see the missing space typing error)

pertinent commets please ;)

With the statement that if the pointers can't be 0 that constructor must be able to throw. Even if 0 was a well defined state it would be an error and the constructor should throw. This means the constructor should be able to throw even if the pointer could be 0.

Did you even read what he says? Do even know what it means to not have exception specifications?

He also said "YouIdiotButWeAreNotLookingAtExceptiontForNow".

The result is undefined behaviour. Even (char*)1 is already undefined because a int doesn't have to be castable into a pointer.

It says that strlen comes from the ISO C standard and C doesn't have exceptions. This implies that it doesn't throw.

So why do you need a macro?

Oh sorry. In that case I'll not answer your questions in future as you seem to already know everything I know.

It feels like a paradox to me. A well defined state is not an error only if U define the 0 state as an error and then yes but this is outruled from the beginning so pointless to talk about.

Yes of course I read what he says, but that doesn't take me the right to disagree with him does it ? Anyway if U take away the throw(exception) from the constructor the example I've shown still works the same...

I didn't quite understand what he ment by that... I'm not a native english speaker but that doesn't seem to void any of my posts... only if he ment that no try/catch should be used which I didn't quite get from that statement.

actually I ment an invalid pointer more like char *p; strlen(p);

I think the standard is not completly clear about that or I didn't understand that...

I know a copy/paste would have ensured that, but that was more like a way of showing how you can avoid typing errors in the contents of the string (it's only usefull if u have many occurences of the same string)

I am sorry that was ment to be a joke, i didn't mean to offend you...
I didn't understand what you ment... sorry it won't happen again....

Herb Sutter:
"An empty throw-specification on a constructor declares to the world that construction cannot fail.
If for whatever reason it can indeed fail, then the empty throw-spec isn't appropriate, that's all."

http://www.gotw.ca/gotw/066.htm (last line)

If I write:

and the constructor is not capabable of constructing a string object with the value "Hello" and instead constructs one with the value "" then it's an error.

Allowing a pointer to be NULL implies that we define a state for that situation. I would find it logical to define the empty string like that but that doesn't have to be. However a NULL pointer must have extacly and only one state. However every constructor except the default constructor is meant to make it possible to construct the object in a different well defined state by the constructor arguments. Falling back to the NULL pointer state in an error situation is an error itself because it does not reflect the state which the programmer who constructs the object wants.

Not failing doesn't only mean to do something but also to that what is requested.

He says that you shouldn't use exception specifications at all so it's best to not use them. Even a lot of compilers don't implement them because they are useless.

That means "You idiot but we are not looking at exceptiont for now" which means as much as "Don't bother with exceptions when answering his questions.". This implies that we should assume that new doesn't fail and thus doesn't throw.

Even then the behaviour is undefined as strlen is only defined for valid, 0 terminated character arrays.

Then don't use the standard as reference as it is pretty clear on this.

I think you didn't understand my question. I asked why you need a macro here not why you used a macro here. You could as well use a const char str[] = "My String" to acheive the same effect. A macro is not needed here.

this is what I have in the standard at 17.4.4.3/2 :(

where can I get your version ?

anyway with all your comments summed up I came up with this:

i also agree that the default value should be reported as error also
but the original poster chose not to...
(I also agree that the C functions should not throw ;), though today I tried it on 3 compilers and they all threw exceptions :angry: )

I also have that there. I mispelled and meant 17.4.4.8/3 sorry. The paragraph is called "Restrictions on exception handling" and can be found under that title in the index.

On which compilers did you try? I doubt that any compiler will throw in that situation. Note that a crash or a Win32 structured exception is not a C++ exception.

For example VC++.NET 2003

You are quite correct I did not take into account an exception in the constructor. As I hoped the value of nameP would suggest. (I am considering writting a tip on it latter).
But I disagree with your solution.

1) Don't use the exception specifications (as pointed out by Dr V).
2) Of course there are exceptions.

OK. So the constructor can throw an exception if it runs out of memory.
Thats fine. If we run out of memory an exception is good. An exceptional thing happened and I would just let the exception propogate.

The bad thing in my code only happens if an exception is thrown during the allocation of value. In this case name would be leaked as the destructor is not called (if an object is not fully constructed the destructor is not called [KTC what is the section in the standard]).

A couple of solutions to this:

1) Do what I should have done wrap each raw pointer in a class.
2) Catch the exception deallocate and rethrow.

Solution 1:
I leave this as an exercise. Any suggestions.
Look at std::auto_ptr<> as an example (though auto_ptr<> can not handle arrays).
Though if you make the class a private member class it need not be as generic as std::auto_ptr<>

Solution 2:

the destructors are still called for the succesfully constructed base classes and the already constructed members...

Yes. But not relavant to this example. As there are no base classes or members with destructors.
That is also why solution 1 above (the prefered solution) suggests wrapping the raw pointers in a class similar to std::auto_ptr<>

I think I got a sollution without creating a wrapper class (but some private member functions):

A lot of work. Not much gain.

Your solution is equivalent to solution 2 above (though much more paranoid about exceptions).

A wrapper class around the raw pointer is still the prefered solution.

That's a VC extension which only works when you are using a cetain exception handling model. strlen doesn't throw a C++ exception but the system stoppes it's execution because it causes an access violation. The system then throws a system exception. VC is capable of catching these exceptions using normal try catch blocks.

For example the following code will not crash using VC and your settings.

However the behaviour is still undefined.

Also what are the 2 other compilers on which you claim that it doesn't crash.

Do you know that:
a) delete doesn't throw
B) deletetion constructs must never throw
c) catching an exception but not handling it defeats the whole purpose of using exceptions
d) delete[] is defined for 0
e) if delete[] should fail the error occured somewhere before because the pointer is invalid and thus is a programming error and should crash

What's the point of delegating the destruction to a method?

Why not?

Borland C++ builder 5 and I used that in some older gcc version too :(

the problem with delete is actually wider... it has problems on many compilers especially the older ones, I remember BC31 could not delete 0...

actually you're right not having any need for that try/catch there but my paranoia didn't invalidate that code ;)

consider that U have a class that needs more than 2 char *
your sollution would end up having many try/catches so that u free all memory after exception...

also spotted a small typing mistake in both constructors:

it should be

but for this case your solution is more elegant ;)

I just tried to give a more general one...

Last time I tried the BCC5.5 compiler is was not capable of catching structured exceptions with a catch(...) block. You need to use the deprecicated __try __except constructions. The newer GCC versions certainly aren't either because they use setjmp and longjmp to implement exception handleing. VC is only capable of doing it when a certain exception handling model is used. Under *nix systems it certainly doesn't work because there are no structured exceptions.

But let use consider it did work. Even in this situation your code would be a death sin. Never catch an exception without handling or rethrowing it.

Prestandard compilers are no argument. Besides BC31 is obsolete. BC5.5 (or newer) is already out which certainly handels this correctly.

But it certainly didn't help it.

Wrapping the char* into it's own class and using composition as myork and I already suggested many times is the solution to this problem.

So? That's no in the specifications and trivial to add although as already said and I'll say it again a bad idea.

Hmm, whenever did I volunteer to become a language lawyer?? <_< :rolleyes:

Now, the link to GotW #66 earlier actually says in the article the very thing you're saying. Namely, only fully constructed (sub)object has its destructor call.

As to the actual standard text, the closest you're getting is the following:

actually that is the way I would do it too (if I'm not allowed to use std::string ;))