Are these c'tors legal or dangerous?

[John E]

I've just come across this code in a 3rd party library which seems otherwise very professionally written....

// Declaration
class CMyClass
{
public:
	CMyClass(void);
	CMyClass(char* name);
	virtual ~CMyClass(void);

// Data
private:
	char* name;

public:
	char* GetName(void) { return name; }
};


// Constructor #1
CMyClass::CMyClass(void)
: name("Default name")
{
}


// Constructor #2
CMyClass::CMyClass(char* szName)
: name(szName)
{
}

The class seems to work but my question is whether or not these c'tors are dangerous. The member function CMyClass::GetName() successfully returns either the supplied name or the default name depending on which c'tor was used - but it worries me that no memory's been allocated anywhere.

C'tor #2 makes reasonable sense. It's c'tor #1 that worries me. Is the string "Default name" guaranteed to exist for the duration of the object?

[RoboTact]

Yes, it's guaranteed. It has static storage duration.
However there are problems with this class.

CMyClass
 c1;
char* str = c1.GetName();
// is it OK to have "Default name" returned?

char buf[10]="abc";
CMyClass c2(buf);
buf[0]='d';
// now c2.GetName() returns "dbc";

char buf2[10]="efg";
CMyClass c3(buf2);
c3.GetName()[0]='h';
// now buf2 is "hfg"

[SuperKoko]

The class seems to work but my question is whether or not these c'tors are dangerous. The member function CMyClass::GetName() successfully returns either the supplied name or the default name depending on which c'tor was used - but it worries me that no memory's been allocated anywhere.

This class doesn't own this character pointer.
I think it's not a very good idea, but if it's written in the class's documentation, it's not incorrect.
The guy who invokes the constructor has to know that the lifetime of the data pointed-at by the pointer, have to be longer than the lifetime of the object.
Here, I think it's not a particulary good idea... A name is something that should be owned by the class.

C'tor #2 makes reasonable sense. It's c'tor #1 that worries me. Is the string "Default name" guaranteed to exist for the duration of the object?

The string "Default name" is guaranteed to exist from the start of the program until the end. No problem.

There are ugly things here:

• Having a default name is not a great idea, and... Since it's a default parameter, it should have been treated as a default parameter, not with an overload of the constructor.
• This program seems const unaware... This name should probably be a std::string or a const char*
• The destructor is very probably uselessly virtual since there is no other virtual function.

[Graham]

Yep. The second ctor is the one that would worry me - no guarantee that the pointed-to string exists for the lifetime of the object. A std::string member would be much safer.

Also, the use of "(void)" in a C++ program is a bit odd. Fine for C, but empty parentheses in C++ mean "no arguments". Putting void in there is pointless and ugly.

[John E]

So let's suppose that I now have a second class which contains a member pointer to the original class (CMyClass* m_pMyClass). Am I right in thinking that this would work:-

CMyOtherClass::CMyOtherClass()
{
	m_pMyClass = new CMyClass("A suitable name");
}


CMyOtherClass::~CMyOtherClass()
{
	delete m_pMyClass;
}


CMyOtherClass::SomeFunc()
{
char buf[32];

	strcpy(buf, m_pMyClass->GetName();
}

whereas this would not work:-

CMyOtherClass::CMyOtherClass()
{
char buf[32] = "A suitable name"

	m_pMyClass = new CMyClass(buf);
}


CMyOtherClass::~CMyOtherClass()
{
	delete m_pMyClass;
}


CMyOtherClass::SomeFunc()
{
char buf[32];

	strcpy(buf, m_pMyClass->GetName();
}

[jfaust]

but my question is whether or not these c'tors are dangerous.

It certainly violates Meyer's Effective C++ Item 18:

"Make interfaces easy to use correctly and hard to use incorrectly"

Jeff

[John E]

Yes - I'm starting to realise that this class will work correctly if it's always constructed using a string literal (whether default or not) but it would be risky to use with a non-const string. Is that right?

I might modify it to use std::string instead of char* name;

[jfaust]

Yes - I'm starting to realise that this class will work correctly if it's always constructed using a string literal (whether default or not) but it would be risky to use with a non-const string. Is that right?

More precisely, the lifetime of the string must exceed the lifetime of the class holding it.

I might modify it to use std::string instead of char* name;

That would be a huge improvement in both safety and correctness.

Jeff

[Yves M]

In your post #6, you are right. The second code leads to undefined results. And basically it would be dangerous to use the class with any string that's not a literal, because you'll have to watch the lifetime of the string vs the lifetime of the object.

I might modify it to use std::string instead of char* name;

By all means. There shouldn't be any problem because the class didn't own the storage in the first place.

[John E]

Just looking through the rest of the 3rd party code I can see that objects of this type are always being created using a string literal but I'll change the class to use std::string anyway. I'm confused by this comment from SuperKoko:-

The destructor is very probably uselessly virtual since there is no other virtual function.

Here's what I always thought to be the received wisdom with regard to virtual d'tors (i.e. always make the base class d'tor virtual).

[Yves M]

Yes, as soon as you use the class as a base in a hierarchy you should have a virtual destructor. Whether or not the class has virtual functions doesn't matter. (Now whether it's good to have a class hierarchy without virtual functions is another story )

[John E]

Suppose you have a 3 class hierarchy - base.... derived1 (derived from base) and derived2 (derived from derived1). Should you put virtual functions in base and derived1 or is it only necessary in the base class?

[RoboTact]

Yes, as soon as you use the class as a base in a hierarchy you should have a virtual destructor. Whether or not the class has virtual functions doesn't matter.

There're many final classes.

[Yves M]

There're many final classes.

Hum, there are no final classes in C++. The language doesn't support it. But anyways, the virtual destructor only applies to classes used "as a base in a hierarchy" as I said previously.

Suppose you have a 3 class hierarchy - base.... derived1 (derived from base) and derived2 (derived from derived1). Should you put virtual functions in base and derived1 or is it only necessary in the base class?

If you don't override virtual functions then the whole concept doesn't make sense. So if you only have virtual functions in the base class, it's not really useful, because you can't actually use polymorphism (i.e. p->somefunc() will always be determined at compile time if you don't override virtual functions). So if you override the virtual function in base and derived1, it makes more sense.

[SuperKoko]

Yes, as soon as you use the class as a base in a hierarchy you should have a virtual destructor. Whether or not the class has virtual functions doesn't matter. (Now whether it's good to have a class hierarchy without virtual functions is another story )

virtual destructors should be used when the object might be destroyed through a pointer to Base.
I highly doubt you'll in your right mind, destroy an object through a pointer to a base class having no virtual function.
Well, you can be paranoid and know that you're dumb and destroy with wrong pointers your objects... But in that case, you'll want to add a virtual destructor to the void type... Unfortunately, that's impossible.

Anyway, since concrete classes are rarely derived from... If you've a class designed to be derived, you can define the destructor as protected.

There is nothing fundamentally wrong with making all destructors virtual... It's not more wrong than making all non-static member functions virtual... But you must know that there are speed and space overheads, and your intent may not be clear for a normal person who would read your code.