Are these c'tors legal or dangerous?

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

Code:

---

// 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?

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

Code:

---

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"

---

Quote:

---

Originally Posted by John E

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.

Quote:

---

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.

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.

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:-

Code:

---

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:-

Code:

---

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();
}

---

Quote:

---

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

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;

Quote:

---

Originally Posted by 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?

---

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

Quote:

---

Originally Posted by John E

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

---

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

Jeff

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.

Quote:

---

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.

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:-

Quote:

---

Originally Posted by 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).

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 ;) )

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?

Quote:

---

Originally Posted by 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.

---

There're many final classes.

Quote:

---

Originally Posted by RoboTact

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.

Quote:

---

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.

Quote:

---

Originally Posted by 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 ;) )

---

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.

Quote:

---

Originally Posted by 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?

---

In general, the base class should contain the initial virtual function declarations and definitions.
Perhaps pure virtual if it is an abstract class, in which case no definition is given.

Then, each derived class should override those virtual functions which it inherits from the base so that polymorphism can be used.

Quote:

---

Originally Posted by 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:-

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).

---

The author of that article is a little confused. Essentially correct, but gives the wrong impression. As SuperKoko and Yves have said - you need a virtual destructor if there is any chance that you will attempt to call delete on a pointer to base that is actually pointing to derived. In a well-designed program, this will only be possible if the base class contains virtual functions, since you will only be holding a pointer to base in order make use of polymorphic behaviour.

Note that an alternative to a public virtual destructor is a protected non-virtual destructor. This simply prevents you from deleting a derived through a pointer to base - you have to delete through a pointer to derived (or you could have a derived declared on the stack - a protected base dtor prevents attempt to call delete if you pass its address around).

Quote:

---

Originally Posted by linked article

There is one more point to be noted regarding virtual destructor. We can't declare pure virtual destructor. Even if a virtual destructor is declared as pure, it will have to implement an empty body (at least) for the destructor.

---

Quote:

---

Originally Posted by dcjr84

In general, the base class should contain the initial virtual function declarations and definitions.
Perhaps pure virtual if it is an abstract class, in which case no definition is given.

---

NO, no and thrice no! "Pure" says nothing about whether the function has a body or not. "Pure" only means that the function is virtual and that its declaration in the class definition includes "= 0". Nothing more. It does not imply that the function has no body. Admittedly, pure virtuals rarely do have bodies, but it is not implicit in the definition of a pure virtual function.

The linked article is wrong - you can declare a pure virtual destructor, but you must give it a body.

Quote:

---

Originally Posted by Graham

NO, no and thrice no! "Pure" says nothing about whether the function has a body or not. "Pure" only means that the function is virtual and that its declaration in the class definition includes "= 0". Nothing more. It does not imply that the function has no body. Admittedly, pure virtuals rarely do have bodies, but it is not implicit in the definition of a pure virtual function.

---

I created an example to test this.

In the first example, I try to define my pure virtual function in the base class.

Code:

---

#include <iostream>
#include <string>
#include <new>

class Shape
{
  public:
      Shape(){;}
      virtual void draw() = 0
      {std::cout << "Hello!" << std::endl;}

  private:
      std::string name;
};

class Circle : public Shape
{
  public:
      Circle(){name = "circle";}
      virtual void draw()
      {std::cout << name << std::endl;}

  private:
      std::string name;
};

class Triangle : public Shape
{
  public:
      Triangle(){name = "triangle";}
      virtual void draw()
      {std::cout << name << std::endl;}

  private:
      std::string name;
};

int main()
{

  Shape* myShapeArray[ 2 ];
 
  myShapeArray[ 0 ] = new Circle();
  myShapeArray[ 1 ] = new Triangle();

  myShapeArray[ 0 ]->draw();
  myShapeArray[ 1 ]->draw();

  return 0; 

}

---

The code does not compile and gives the following error

Quote:

---

10 C:\Documents and Settings\dcjr84\My Documents\Untitled1.cpp pure-specifier on function-definition

---

Now, in this example, I have the definition commented out.
Everything works fine as it should....

Code:

---

#include <iostream>
#include <string>
#include <new>

class Shape
{
  public:
      Shape(){;}
      virtual void draw() = 0;
      //{std::cout << "Hello!" << std::endl;}

  private:
      std::string name;
};

class Circle : public Shape
{
  public:
      Circle(){name = "circle";}
      virtual void draw()
      {std::cout << name << std::endl;}

  private:
      std::string name;
};

class Triangle : public Shape
{
  public:
      Triangle(){name = "triangle";}
      virtual void draw()
      {std::cout << name << std::endl;}

  private:
      std::string name;
};

int main()
{

  Shape* myShapeArray[ 2 ];
 
  myShapeArray[ 0 ] = new Circle();
  myShapeArray[ 1 ] = new Triangle();

  myShapeArray[ 0 ]->draw();
  myShapeArray[ 1 ]->draw();

  return 0;

}

---

The output is as expected

Quote:

---

circle
triangle

---

Care to explain this?

From this example it would seem that a pure virtual function does in fact imply no definition, because my code will not compile if I try to give it one.

Quote:

---

Originally Posted by dcjr84

From this example it would seem that a pure virtual function does imply no definition, because my code will not compile if I try to give it one.

---

However, you're wrong.
It's not possible to declare & define at the same time a pure virtual function (probably due to a parsing limitation).
But if you define your function separately, it should work!

Code:

---

class Shape
{
  public:
      Shape(){;}
      virtual void draw() = 0;

  private:
      std::string name;
};
void Shape::draw() {
std::cout << "Hello!" << std::endl;
}

---

Note that, even if a definition is provided, calling a pure virtual function without explicit scope resolution, has undefined behavior.

Quote:

---

Originally Posted by SuperKoko

It's not possible to declare & define at the same time a pure virtual function (probably due to a parsing limitation).

---

Aha!

Indeed, when I define the pure virtual function separately, it compiles fine and works properly.

I was wrong, my mistake :D

Apologies if I get a bit het up over this issue, but every single candidate I interview gets it wrong. Although it's rarely used, it is very useful. Basically, what you are saying by giving an implementation for a pure virtual function is "here is a default implementation for this function. If a derived class wishes to accept the default it must do so explicitly by overriding the function and calling the base class version". When you have a non-pure virtual function, the derived class can get the default action by accident. A pure with implementation eliminates the accidental inheritance of implementation.

Quote:

---

Originally Posted by Graham

Apologies if I get a bit het up over this issue, but every single candidate I interview gets it wrong. Although it's rarely used, it is very useful. Basically, what you are saying by giving an implementation for a pure virtual function is "here is a default implementation for this function. If a derived class wishes to accept the default it must do so explicitly by overriding the function and calling the base class version". When you have a non-pure virtual function, the derived class can get the default action by accident. A pure with implementation eliminates the accidental inheritance of implementation.

---

Good point, but the same could be done with ordinary default implementation method (declared separately) plus pure virtual method. All without these arcane pure-virtual-with-implementation things. You don't need surprising language feature in order to do this. Especially if as you admit it's rarely used.

Quote:

---

Originally Posted by RoboTact

Good point, but the same could be done with ordinary default implementation method (declared separately) plus pure virtual method. All without these arcane pure-virtual-with-implementation things. You don't need surprising language feature in order to do this. Especially if as you admit it's rarely used.

---

a.k.a. Template Pattern or Non-Virtual Interface. I prefer these mechanism as they are more likely to be used correctly from the derived class.

Jeff

Quote:

---

Originally Posted by RoboTact

Good point, but the same could be done with ordinary default implementation method (declared separately) plus pure virtual method. All without these arcane pure-virtual-with-implementation things. You don't need surprising language feature in order to do this. Especially if as you admit it's rarely used.

---

Maybe it's because our clocks have gone back, but I don't see how they're the same. If you provide an unimplemented pure virtual, then the derived class has to provide an implementation of its action. What I'm saying is that the derived implementation consists solely of a call to the base function:

Code:

---

class foo
{
public:
        virtual void f() = 0;
};

void foo::f()
{
        // implementation
}

class bar : public foo
{
public:
        virtual void f()
        {
                foo:f();
        }
};

---

Are you suggesting something like:

Code:

---

class foo
{
public:
        virtual void f() = 0; // no implementation

protected:
        void f_default()
        {
                // default implementation
        }
};

class bar : public foo
{
public:
        virtual void f()
        {
                foo::f_default();
        }
};

---

If so, I don't see where the gain is.

And pure-with-implementation is not arcane. It's simply understanding what a pure virtual function actually is - it's a virtual function that must be overridden, nothing more, nothing less.

Code:

---

class foo
{
public:
        void f()
        {
                // default implementation
                ...
                handleF();
        }
 

protected:
        // alternatively, could be pure virtual
        virtual void handleF()
        {
        }
};

class bar : public foo
{
protected:
        virtual void handleF()
        {
                // derived implementation
        }
};

---

Jeff

Quote:

---

Originally Posted by jfaust

Code:

---

class foo
{
public:
        void f()
        {
                // default implementation
                ...
                handleF();
        }
 

protected:
        // alternatively, could be pure virtual
        virtual void handleF()
        {
        }
};

class bar : public foo
{
protected:
        virtual void handleF()
        {
                // derived implementation
        }
};

---

Jeff

---

It isn't equivalent to a default implementation, since the code around in f() is executed even if handleF() is overriden.
Yes, it is often wishable and useful, but it doesn't solve the same problem.

Quote:

---

Originally Posted by SuperKoko

It isn't equivalent to a default implementation, since the code around in f() is executed even if handleF() is overriden.
Yes, it is often wishable and useful, but it doesn't solve the same problem.

---

Not precisely the same problem, but there is an overwhelming amount of overlap. In the cases where you don't want to execute the default code, you could modify my example to handle the same problem:

Code:

---

class foo
{
public:
        void f()
        {
                assert(preconditions);
                defaultF();
                handleF();
                assert(postconditions);
        }
 

protected:
        virtual void defaultF()
        {
                // default implementation
        }

        // alternatively, could be pure virtual
        virtual void handleF()
        {
        }
};

class bar : public foo
{
protected:
        virtual void handleF()
        {
                // derived implementation
        }
};

---

This would be safer for the cases where most derived classes do want the default, but still leave room for others to prevent the default.

Note that I'm not arguing against pure virtuals with implementations. This is just a technique that I use in similar circumstances. One reason I really like this mechanism is the assertions I've put in the non-virtual method.

Jeff

jfaust: yes, your example is one that I use myself, probably more often than I use PVI (pure-virtual-with-implementation). However, I agree with SuperKoko that it does not solve the same problem.

Template pattern gives you a mandatory implementation, with an overridable part.

POV (plain ol' virtual) gives you an optional implementation that can be inherited accidentally.

PVI gives the same as POV, but without the "accidental" part.

POV and PVI are subsets of template pattern, not synonyms, which is why there is so much overlap, as you point out.

Indeed, Template pattern is, I would guess, the primary place for using PVI - you make the variable parts of the template PVI functions, so that a derived class has to explicitly accept the default processing at each of those points.

The problems I sometimes have with virtual functions is that I get the override prototype slightly wrong (maybe missing a const or putting one in where there shouldn't be one, or a slightly different name or parameter list) and then I end up not with an override at all but with a totally different function that is in reality never invoked.

A good reason, of course, to use pure virtual functions even when they have a default implementation. Then these errors would be caught at compile time.

Yves: you are not quite correct. There is a way to finalise a class in C++ - give it all private constructors and a friend or static method to create one. Then there is no way to derive from the class.

What is not possible is to finalise a single virtual method within the class. If you really do want to do that (you want to particular implementation where it is different functions that are overloaded) then you can split the class in two, have a class with private constructors but with a "has-a" relationship on your new interface so that others will derive from your new interface and pass a pointer (or smart-pointer) as a parameter to your constructor.