Wade Not In Unknown Waters: Part One

Published August 26, 2013 by Andrey Karpov, posted by Code_Analysis
Do you see issues with this article? Let us know.
Advertisement

We decided to write several small posts on how C/C++ programmers play with fire without knowing it. The first post will be devoted to an attempt to explicitly call a constructor.

Programmers are lazy creatures. That's why they tend to solve a task using minimal amounts of code. This aim is praiseworthy and good. But the main point is to not get too involved in the process and stop at the right time.

For example, programmers are too lazy to create a single initialization function in a class so that it could be called from various constructors later. They think: "What do I need an extra function for? I'd rather call one constructor from the other". Unfortunately, sometimes programmers can't solve even such a simple task. It is to detect such unsuccessful attempts that I'm implementing a new rule in PVS-Studio. Here is, for instance, a code sample I have found in the eMule project:

class CSlideBarGroup { public: CSlideBarGroup(CString strName, INT iIconIndex, CListBoxST* pListBox); CSlideBarGroup(CSlideBarGroup& Group); ... } CSlideBarGroup::CSlideBarGroup(CSlideBarGroup& Group) { CSlideBarGroup( Group.GetName(), Group.GetIconIndex(), Group.GetListBox()); }

Let's examine more attentively how the last constructor is implemented. The programmer decided that the code

CSlideBarGroup( Group.GetName(), Group.GetIconIndex(), Group.GetListBox());

simply calls the other constructor. Nothing of the kind. A new unnamed object of the CslideBarGroup type is created and destroyed right afterwards.

It appears that the programmer has actually called the other constructor. But he/she has not quite done the same thing he/she intended: the class fields remain uninitialized.

Such errors are just half the trouble. Some people do know how to call the other constructor really. And they do it. I wish they didn't know :)

For instance, the above given code could be rewritten in this way:

CSlideBarGroup::CSlideBarGroup(CSlideBarGroup& Group) { this->CSlideBarGroup::CSlideBarGroup( Group.GetName(), Group.GetIconIndex(), Group.GetListBox()); }

or in this way:

CSlideBarGroup::CSlideBarGroup(CSlideBarGroup& Group) { new (this) CSlideBarGroup( Group.GetName(), Group.GetIconIndex(), Group.GetListBox()); }

Now one data initialization constructor is really calling the other constructor.

If you see a programmer doing this, deal him/her one flick on his/her forehead for yourself and one more flick on my behalf. The cited examples contain very dangerous code and you should understand well how they work! Being written for the purpose of petty optimization (programmers are too lazy to write a separate function), this code might do more harm than good. Let's see more closely why such constructs sometimes work but most often don't.

class SomeClass { int x,y; public: SomeClass() { new (this) SomeClass(0,0); } SomeClass(int xx, int yy) : x(xx), y(yy) {} };

This code will work correctly. It is safe and works well, since the class contains primary data types and is not a descendant of other classes. In this case, a double constructor call is harmless.

Let's consider another example where an explicit constructor call causes an error (the sample is taken from this discussion on the StackOverflow website):

class Base { public: char *ptr; std::vector vect; Base() { ptr = new char[1000]; } ~Base() { delete [] ptr; } }; class Derived : Base { Derived(Foo foo) { } Derived(Bar bar) { new (this) Derived(bar.foo); } }

When we call the new (this) Derived(bar.foo); constructor, the Base object is already created and fields initialized. The repeated constructor call will cause a double initialization. A pointer to the newly allocated memory area will be written into ptr. As a result, we get a memory leak. The result of double initialization of an object of the std::vector type cannot be predicted at all. But one thing is obvious: such code is inadmissible.

Conclusion

An explicit constructor call is needed only in very rare cases. In common programming practice, an explicit constructor call usually appears due to a programmer's wish to reduce the code's size. Don't do that! Create an ordinary initialization function.

This is how the correct code should look:

class CSlideBarGroup { void Init(CString strName, INT iIconIndex, CListBoxST* pListBox); public: CSlideBarGroup(CString strName, INT iIconIndex, CListBoxST* pListBox) { Init(strName, iIconIndex, pListBox); } CSlideBarGroup(CSlideBarGroup& Group) { Init(Group.GetName(), Group.GetIconIndex(), Group.GetListBox()); } ... };

P.S. Explicit call of one constructor from the other in C++11 (delegation)

The new C++11 standard allows you to perform the call of constructors from other constructors (known as delegation). It enables you to create constructors that use the behavior of other constructors without added code. This is an example of correct code:

class MyClass { std::string m_s; public: MyClass(std::string s) : m_s(s) {} MyClass() : MyClass("default") {} };
Cancel Save
0 Likes 11 Comments

Comments

jbadams

Unless I'm missing something -- perhaps you're talking about some extended version of the language or new standard -- I don't believe C has constructors. You should probably remove the "c lang" tag and reference to C in the introduction paragraph. Based on this I've voted to reject the article for now, citing that it is "inaccurate/misleading", but I'll be happy to remove that vote if the problem is resolved. smile.png

EDIT: Removed comment about advertising, as I don't feel it's excessive, and it appears there may be follow-up articles without any advertising links.

Satisfactorily resolved. smile.png

July 18, 2013 02:18 PM
jjd
jjd

I think there is actually an important element missing from this article. There is a good reason for trying to re-use the constructors, and it has nothing to do with being lazy. Having explicit init() functions often leads to objects that can be created in an incomplete state. This is undesirable although sometime unavoidable. When delayed initialization can be avoided, it should be.

I think at this article would be more effective if you directly addressed the issue of delayed initialization; Show why using new (this) foo is a dangerous pattern, and elevate the solution provided by C++11 to more than simply a footnote.

July 18, 2013 02:59 PM
Servant of the Lord

It only has one link to PVS-Studio, and I don't think it's too heavy-handed of an ad.

Although I think alot of people trying to call constructors from constructors aren't being lazy, but just don't know that it doesn't work (obviously if they knew it didn't work, they wouldn't do it).

Maybe they come from languages where that's allowed.

July 18, 2013 04:45 PM
4thworld

"Having explicit init() functions often leads to objects that can be created in an incomplete state. This is undesirable although sometime unavoidable."

This is so true ! Also when the init function is kept private.

sorry can anyone explain me this type of syntax?

new (this) Derived(bar.foo);

it's the first time i see that!

July 18, 2013 05:19 PM
adam4813

new () is the placement new operator. It is the same as calling the new operator, but you provide the placement (or address) at which to create it. Hence calling it with the this pointer tells the new operator to place the created object at the same location as the current.

However that could be bad if you use placement new on the this pointer when you use a base class constructor, The base class isn't as large as the derived type in memory and thus you will leak memory and have some odd/undefined behavior.

July 18, 2013 08:44 PM
Oberon_Command
Why no mention of calling the superclass constructor in the initializer list? You can do that in C++, can you not? Would that not be a better option that solves the problems constructor chaining is meant to solve?
July 19, 2013 12:57 AM
adam4813

The article is referring to calling alternative constructors within the same class.

An example would be calling Car() with a Truck object as an argument. The Car constructors might be Car(year, make, model, speed, tank_size) and Car(year, make, model, style). However it may have one that would take a Truck Car(Truck*) and make a similar car using the Trucks data (year, speed, tank_size). So you would call Car(Truck*, make, model) and that constructor would then call Car(truck.year, make, model, truck.speed, truck.tank_size)

July 19, 2013 03:02 AM
Code_Analysis

Unless I'm missing something -- perhaps you're talking about some extended version of the language or new standard

This article is about C++. "We decided to write several small posts on how C/C++" - I mean, there will be several articles. Some will be about C++, the other about C/C++.

July 22, 2013 10:23 AM
ExcessNeo

I think you should perhaps consider moving the small section on delegating constructors up above the conclusion as it could be quite easily missed and arguably will result in less people using initialisation functions if their compiler supports delegation.

July 22, 2013 03:32 PM
Adam_42

Is there a compiler for which this function actually compiles given a valid class definition? I couldn't find one...

CSlideBarGroup::CSlideBarGroup(CSlideBarGroup& Group)
{
this->CSlideBarGroup::CSlideBarGroup(
Group.GetName(), Group.GetIconIndex(), Group.GetListBox());
}

August 05, 2013 11:20 PM
Ronnie Mado Solbakken

Tbh though, I doubt that it's possible to wade in those waters. :D

August 26, 2013 05:02 PM
You must log in to join the conversation.
Don't have a GameDev.net account? Sign up!

We decided to write several small posts on how C/C++ programmers play with fire without knowing it. The first post will be devoted to an attempt to explicitly call a constructor.

Advertisement
Advertisement