• Advertisement
Sign in to follow this  

abstract base class and clone

This topic is 4142 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I've been wrestling with abstract base class and a clone method, but can't make it work without sacrificing type-safety. Here's the problem:
class Abstract {
public:
   virtual void clone(const Abstract &A) = 0;
};

class Concrete : public Abstract {
private:
   int i;

public:
   void clone(const Concrete &C) {...}
   //this function is considered by the compiler to be different from the virtual function in Abstract

   void clone(const Abstract &A) {...}
   //in this function, A needs to be cast to Concrete in order to access i, which is not type-safe
   //also, it accepts an Abstract object from the user, which shouldn't be allowed
};


Please help.

Share this post


Link to post
Share on other sites
Advertisement
search your compiler documentation and enable C++ RTTI (Run Time Type Info) - be warnedm, this adds additional overhead to each class and needs to be carefully used.

simple example code:

#include <string>
#include <exception>
#include <iostream>



class Abstract {
public:
virtual void clone(const Abstract &A) = 0;
};


class cast_exception : public std::exception
{
std::string from, to;
public:
virtual ~cast_exception() throw() {}

cast_exception( std::string from_, std::string to_ ) : from(from_), to(to_) {}
virtual const char* what() const throw() {
return (std::string("Class Cast Exception, Tried to cast from ")
+ from + std::string(" to ") + to).c_str();
}
};


class Concrete : public Abstract {
private:
int i;

public:

Concrete(int i_) : i(i_) {}

//void clone(const Concrete &C) {...}
//this function is considered by the compiler to be different from the virtual function in Abstract

void clone(const Abstract &A)
{
const Concrete* c_ptr = dynamic_cast< const Concrete* >( &A );
if( c_ptr == NULL ) {
throw cast_exception("Abstract", "Concrete");
}

this->i = c_ptr->i;
}


const int get() const { return i; }
};


class DifferentConcrete : public Abstract {
public:
float j;
DifferentConcrete( float j_ ) : j(j_) {}
const float get() const { return j; }

virtual void clone(const Abstract &A) {}
};


int main()
{
Concrete *c = new Concrete(25);
Concrete *b = new Concrete(50);
DifferentConcrete *d = new DifferentConcrete(23.32f);

try {
b->clone(*c);
b->clone(*d);
} catch( std::exception& e ) {
std::cout << e.what() << std::endl;
}

return 0;
}

Compile with g++ -W -Wall filename.cpp

Share this post


Link to post
Share on other sites
I had thought clone methods were implemented like this:

class Abstract
{
public:
virtual Abstract * clone() = 0;
};

class Concrete1 : public Abstract
{
public:
virtual Abstract * clone() { return new Concrete1(*this); }
};

class Concrete2 : public Abstract
{
public:
virtual Abstract * clone() { return new Concrete2(*this); }
};

Abstract * p1;
Abstract * p2 = new Concrete1;
Abstract * p3 = new Concrete2;
p1 = p2->clone(); // p1 now points to a Concrete1 object
delete p1;
p1 = p3->clone(); // p1 now points to a Concrete2 object
delete p2;



This way, type safety is preserved.

Share this post


Link to post
Share on other sites
Quote:
Original post by snooty
I've been wrestling with abstract base class and a clone method, but can't make it work without sacrificing type-safety.
Here's the problem:

*** Source Snippet Removed ***

Please help.


You seems to be confused about the prototype pattern (the name of this design pattern). This is because you try to clone the function parameter, while you should clone the current object. Ie instead of writing p3 = p1->clone(*p2), you should write p3 = p2->clone(). This way, as Rainault spotted, you only care about the type of p2 - and this is where the virtual function comes to help, because the type of p2 is known in when cloning.

HTH,

Share this post


Link to post
Share on other sites
Thank you for the replys.

Actually, what I want is for the Concrete class to have a copy constructor. My way of doing it is to call the clone method in the copy constructor to do the copying.

The RTTI way looks ok, I'll try it. But if it is causing a serious performance hit, I think I'll resort to commenting to warn users about this instead. Probably a bad way, but may be the only way...

Share this post


Link to post
Share on other sites
Quote:
Original post by snooty
Thank you for the replys.

Actually, what I want is for the Concrete class to have a copy constructor. My way of doing it is to call the clone method in the copy constructor to do the copying.

The RTTI way looks ok, I'll try it. But if it is causing a serious performance hit, I think I'll resort to commenting to warn users about this instead. Probably a bad way, but may be the only way...


You won't be able to call a virtual function from a constructor. The vtable is not yet defined, meaning that you'll always end up by calling the function which is defined in the class itself (in most implementations of the standard). However, I really strongly suggest you to rethink about this approach, as it is potentially dangerous.

Moreover, cloneing is not copying, as far as the semantic goes. It is perfectly safe to have a private, non virtual copy() function that will do the work for you (and that can be reused in operator=()).

To finish, constructor should use an initialization list everytime it is possible. It is less computationally intensive, as the variable members are initialized at construction, instead of being initialized by a call to their operator=. Consider the case where member a of type A is a complex type, doing:
class B
{
A a;
public:
// a is created and initialized in one operation
B(const B& b) : a(b.getA()) { }
const A& getA() const { return a; }
};

vs.
class B
{
A a;
public:
// call the default constructor of A, then operator=()
// that's slower than the previous code.
B(const B& b) { a = b.getA(); }
const A& getA() const { return a; }
};

My advice is to not try to do fancy stuff here. No offense intended but you seems to show a lack of knowledge regarding how C++ works, so in your position I'd do the simplest thing that can possibly work - which in your case is a basic implementation of the copy constructor.

HTH,

Share this post


Link to post
Share on other sites
I'm very sorry about this, but I don't understand why calling virtual functions in constructors is problematic. Maybe I wasn't expressing myself clearly. Maybe I did a bad job abstracting what I am really trying to do.

So, please allow me to clarify the whole thing, this time using the real program that I'm trying to write.

I want to write a library that contain classes that handle 2D drawing on bitmaps. I use templates to handle different color formats of the bitmap:

enum FORMAT { F555, F888 };

template<enum FORMAT f>
class Image {
private:
UINT *bits; //an array that stores the bitmap bits
};




Everything is fine so far, but then I want to write a class that handles a bitmap compatible to the display format, which is determined at runtime. I let the former Image class derive from an abstract base class so that the new DisplayImage class can store a pointer to it:

class Base { };

template<enum FORMAT f>
class Image : public Base {
private:
UINT *bits;
};

class DisplayImage : public Base {
private:
Base *img;
};




Now, both Image and DisplayImage have a copy constructor and a copy method:

class Base {
public:
virtual void Copy(const Base &b) = 0;
};

template<enum FORMAT f>
class Image : public Base {
public:
void Copy(const Base &b) { ... } //PROBLEM: accepts Image<F555> and Image<F888>,
//but an instance of Image<F555> shouldn't accept Image<F888>, and vice versa
Image(const Image &i) { Copy((const Base)i); } //PROBLEM: calling virtual function in constructor, ???
private:
UINT *bits;
};

FORMAT format = F888; //set before DisplayImage is constructed

class DisplayImage : public Base {
public:
void Copy(const Base &b) { img->Copy( ((const DisplayImage)b).img ); } //PROBLEM: using non-typesafe cast
DisplayImage(const DisplayImage &di) {
switch (format) {
case F888: img = new Image<F888>(*(Image<F888> *)di.img); break; //PROBLEM: using non-typesafe cast
case F555: img = new Image<F555>(*(Image<F555> *)di.img); break; //PROBLEM: using non-typesafe cast
}
}

private:
Base *img;
};




Please don't give me up. :)

[Edited by - snooty on October 18, 2006 1:49:02 PM]

Share this post


Link to post
Share on other sites
Is an inheritance hierarchy really necessary here? I don't get the point of some of your classes. What does Base represent? What's the difference between Image and DisplayImage? Why does DisplayImage have a pointer to a Base? Give your justifications for these classes; you may need to do a little class redesign. If the only purpose of Base is to provide a pure virtual Copy method, I don't think you really need inheritance here.

Share this post


Link to post
Share on other sites
Quote:

I'm very sorry about this, but I don't understand why calling virtual functions in constructors is problematic.


Because it just doesn't work. During construction, the type of the object is the type of class for which the current constructor is executing. Virtual dispatch within a constructor of type T cannot dispatch to anything "below" type T in the inheritance hierarchy -- the object is not that type yet, regardless of whether or not it ultimately will be (and because of that, many compilers will emit code to directly call the appropriate methods rather than attempt to perform a v-table dispatch, because the result of the dispatch in known at compile time).

It's not hard see why this is true. If dispatch were "allowed" to happen in a constructor, it still wouldn't work the way you wanted it to because you would be calling a method of a derived type that read or manipulated data members that, since none of the derived type's constructors had run yet, were not even initialized. It would thus be impossible to guarantee class invariants in any virtual function, rendering them so dangerous as to be completely useless, especially for production code.

EDIT:
So, after looking over your code, I raise the same questions as Rainault.
It seems to me that all you need is:

template< FORMAT F_ >
class Image
{
public:
// copy ct
Image(const Image &other)
{
/* implementation here... */
}
};


Given the additional context you've provided here, I don't actually see a need for any kind of inheritance whatsoever. Base is useless. Holding on to pointers to Base in the derived classes is pointless. DisplayImage is useless. I'm not neccessarily sure there's even any benefit to making the format a template argument... but I'll let that slide.

Can you explain, in a larger context, what these classes are for and how they are used in a program?

[Edited by - jpetrie on October 18, 2006 2:25:25 PM]

Share this post


Link to post
Share on other sites
RTTI is totally not called for here.

Quote:
Original post by snooty
Actually, what I want is for the Concrete class to have a copy constructor. My way of doing it is to call the clone method in the copy constructor to do the copying.


As noted, a virtual function isn't going to work properly from within a constructor, because basically, the stuff that makes virtual functions... well... be virtual hasn't happened yet.

Besides which, clone() normally allocates a new object on the heap. If you implemented the copy constructor in terms of clone(), you'd need to assign the data members back from the allocated object, and then delete that allocated object. And if you were then hoping to implement assignment with the copy-and-swap idiom, well... (oops, copying means using the copy constructor... hello infinite recursion) Basically, you can't really clone "into" an existing object, because that's not what "clone" means. Meanwhile, a constructor can only operate on an already-allocated (whether on the heap or the stack) chunk of memory. It isn't responsible for the actual allocation; it's only responsible for initialization (setup).

The real solution is to realize (which is strongly hinted at by the previous paragraph) that you have *this* part backwards, too. :) That is to say: instead, use the copy constructor to implement clone().

Also note that you can vary the *return type* for such a clone function according to what is actually returned, which gives a bit more type safety.

The result looks like:


class Base {
// other stuff
public:
Base(const Base& b) {
// do whatever's needed here (or better yet, in the initialization list)
}
// Now we implement clone() in terms of the copy ctor:
virtual Base* clone() { return new Base(*this); }
};

class Derived {
// Similarly:
public:
Derived(const Derived& d) {
// blah blah...
}
Derived* clone() { return new Derived(*this); }
}

// ...
Base* foo = createBaseOrDerived();
Base* bar = foo.clone(); // now points at a separate object
bar->action();
// will behave polymorphically, with the same result as foo->action()
Derived* baz;
Derived* quux = baz.clone(); // no need to cast now


If the 'Derived* clone' bit doesn't work for you, then upgrade your compiler ;) Or as a workaround, just return Base*, and use static_cast when you create quux.

Reference material.

Share this post


Link to post
Share on other sites
There are many other methods in Image and DisplayImage, like drawing shapes, blitting, text rendering, etc.

So, for the users, if they want to make an Image of format F555, they do
Image<F555> img555;
Or
Image<F888> img888;
if they want an F888 image.
(template is used because the vast majority of the algorithms for Image<F555> and Image<F888> are the same, except for some minor things)

And then, if they want to make an Image that is compatible to the display color format, they do
DisplayImage dimg;
which will provide the same interface as Image<F555> and Image<F888>.

DisplayImage holds a pointer to Base because it creates either an Image<F888> or an Image<F555> according to the display format, and then it forwards all methods (including the Copy method) to this member pointer to carry out the tasks.

class Base {
public:
virtual void DrawLine(...) = 0;
//other methods
};

template<enum FORMAT f>
class Image : public Base { //here Image inherits from Base for 2 reasons:
//1. it has the interface of Base
//2. to provide a way of access for DisplayImage
public:
void DrawLine(...) { /*drawing algorithm*/ }
//other methods
private:
UINT bits;
//other member data
};

class DisplayImage : public Base { //here DisplayImage inherits from Base because it has the interface of Base
public:
void DrawLine(...) { img->DrawLine(...); } //forward to the appropriate virtual functions
//other methods, most of them do the same forwarding as DrawLine
private:
Base *img; //either an Image<F555> or Image<F888>
};



I don't really need a clone method, that was a mistake in the choice of name. I really just want a copy method that the user can call when she wants to copy one Image to another, like:

Image<F555> img1;
Image<F555> img2;
//do something
img1.Copy(img2); //here, img1 will be reset
//then turned into the same Image as img2


Or

DisplayImage dimg1;
DisplayImage dimg2;
//do something
dimg1.Copy(dimg2);


Similar things happen with the copy constructor:

Image<F555> img2;
//do something
Image<F555> img1(img2); //I guess this could be done in the clone way


Hope this design isn't stupid.

[Edited by - snooty on October 18, 2006 10:56:37 PM]

Share this post


Link to post
Share on other sites
I don't think inheritance is justified if your only reason for having it is because two classes share the same interface. You should be able to accomplish what you want without inheritance. Also, unless you anticipate an arbitrary number of Image format types other than the two you listed above, I recommend dropping the template argument; instead, in the Image constructor, pass in the Image type you want and store it into a member variable. A simple if statement can change your algorithms based on the Image format.

class Image {
public:
Image(FORMAT initF) : f(initF) {}
void DrawLine(...) { /*drawing algorithm*/ }
//other methods
private:
UINT bits;
FORMAT f;
//other member data
};

class DisplayImage {
public:
void DrawLine(...) { img->DrawLine(...); } //forward to the appropriate virtual functions
//other methods, most of them do the same forwarding as DrawLine
private:
Image *img;
};


Quote:
I don't really need a clone method, that was a mistake in the choice of name. I really just want a copy method that the user can call when she wants to copy one Image to another

That is the purpose of a clone method. Call it what you will, but a clone method (also called the Prototype design pattern, as described above) is used when you want to have a "virtual copy constructor". In other words, you want a copy of a polymorphic type without having to specify what subtype that object is until runtime. Of course, by eliminating the inheritance, you also eliminate the need for a virtual copy constructor, so a regular copy constructor will do just fine here.

Share this post


Link to post
Share on other sites
Quote:
Original post by Rainault
I don't think inheritance is justified if your only reason for having it is because two classes share the same interface. You should be able to accomplish what you want without inheritance. Also, unless you anticipate an arbitrary number of Image format types other than the two you listed above, I recommend dropping the template argument; instead, in the Image constructor, pass in the Image type you want and store it into a member variable. A simple if statement can change your algorithms based on the Image format.


I don't think it'd work without template. I want to support at least 3 formats. Introducing if's or switch's will result in a performance hit because these switches would be inside inner loops. Also, the type of the bits member data is dependent on the format (UINT for F888, WORD for F555, etc).

Quote:
That is the purpose of a clone method. Call it what you will, but a clone method (also called the Prototype design pattern, as described above) is used when you want to have a "virtual copy constructor". In other words, you want a copy of a polymorphic type without having to specify what subtype that object is until runtime. Of course, by eliminating the inheritance, you also eliminate the need for a virtual copy constructor, so a regular copy constructor will do just fine here.


Now I see the clone method is the correct solution for the copy constructor. But I still want a Copy method that can stand on its own.

Share this post


Link to post
Share on other sites
Quote:

I don't think it'd work without template. I want to support at least 3 formats. Introducing if's or switch's will result in a performance hit because these switches would be inside inner loops. Also, the type of the bits member data is dependent on the format (UINT for F888, WORD for F555, etc).

And templates help you achieve this how? You'll need specializations, at some level, of the class to change the types of its data members and to change the functionality of its methods.

If anything, this is actually a case to use inheritance (of an abstract interface) and virtual methods rather than templates. However, then you are left with the polymorphic copy issue again. In reality, I don't think you need either.

You can do what just about every other image class in the word does, and store a pointer to an unsigned char for the actual image data. This will allow you to interpret it any way you like. For example:


class Image
{
public:
Image(FORMAT f,size_t width,size_t height)
: mFormat(f),mImageWidth(width),mImageHeight(height)
{
mPixelSize = /* determine based on mFormat, for example
F888 is 24, F555 is 16 (1 bit wasted), etc */

mData = new unsigned char[mImageWidth * mImageHeight* mPixelsize];
}

private:
FORMAT mFormat;
unsigned char *mData; // Actual data.
size_t mPixelSize; // In bytes.
size_t mImageWidth; // In pixels.
size_t mImageHeight; // In pixels.
};


Now, you might think you'd run into problems efficiently performing some iterative operations over the entire image that require per-component access -- you might think you'd need a bunch of if or switch statements inside the inner loops of those algorithms.

You'd be wrong.

Based on the format, you construct a structure identifying per pixel or per-bit offsets and masks (whichever is most appropriate for your data set) for each color component, and you store that information with the image. You use it when iterating so you don't need to write code that explicitly knows where each color component is; this is far preferable to having to write a template specialization for every format (which, by the way, means you might as well be writing three separate classes anyway) that duplicates a bunch of common code and differs only by some offsets.

Quote:

Now I see the clone method is the correct solution for the copy constructor. But I still want a Copy method that can stand on its own.

With the syntax of "destinationImage.Copy(sourceImage)" to copy from sourceImage to destinationImage? This is easy enough to implement (though, the name and semantics combined make it sound strange):

void Image::Copy(const Image &source)
{
if(this == &source)
return; // Handle self-copy.

if(mData)
delete[] mData;

mPixelSize = source.GetPixelSize();
mImageWidth = source.GetImageWidth();
/* et cetera */

/* allocate memory based on members, copy from source data
to this object's data. */
}


You can implement your assignment operator in terms of this function... but again, since you no longer need the inheritance, you really don't need this. Better, I think, to stick with more idiomatic methods of copying (operator=).

Quote:

And then, if they want to make an Image that is compatible to the display color format, they do
DisplayImage dimg;

This is horribly broken, and you only need to do it because since you Image classes are templated, you can't make runtime determinations of the template parameters. By eliminating the templates(1) you've eliminated the need for the (rather brittle and unreliable) method of making DisplayImage be the "correct" format for display compatibility by gating it on a global that you hope gets initialized before somebody tries to create a DisplayImage (you can't really control this).

The better solution is to have your rendering interface (which is what knows about which display format is valid for the current rendering setup) have a method called "GetCompatibleImage()" or something that returns an Image that has been appropriately constructed. This way you avoid the global fiasco.

(1) As I've hinted at before, templates make it harder for you. You'll need to specialize the class for each format in order to change the type of the data member, which means you're really just writing three classes since specialization is not an inheritance relationship and you must reimplement the entire class for the specialization.

Share this post


Link to post
Share on other sites
Quote:
Original post by jpetrie
And templates help you achieve this how? You'll need specializations, at some level, of the class to change the types of its data members and to change the functionality of its methods.

If anything, this is actually a case to use inheritance (of an abstract interface) and virtual methods rather than templates. However, then you are left with the polymorphic copy issue again. In reality, I don't think you need either.

You can do what just about every other image class in the word does, and store a pointer to an unsigned char for the actual image data. This will allow you to interpret it any way you like. For example:


class Image
{
public:
Image(FORMAT f,size_t width,size_t height)
: mFormat(f),mImageWidth(width),mImageHeight(height)
{
mPixelSize = /* determine based on mFormat, for example
F888 is 24, F555 is 16 (1 bit wasted), etc */

mData = new unsigned char[mImageWidth * mImageHeight* mPixelsize];
}

private:
FORMAT mFormat;
unsigned char *mData; // Actual data.
size_t mPixelSize; // In bytes.
size_t mImageWidth; // In pixels.
size_t mImageHeight; // In pixels.
};


Now, you might think you'd run into problems efficiently performing some iterative operations over the entire image that require per-component access -- you might think you'd need a bunch of if or switch statements inside the inner loops of those algorithms.

You'd be wrong.

Based on the format, you construct a structure identifying per pixel or per-bit offsets and masks (whichever is most appropriate for your data set) for each color component, and you store that information with the image. You use it when iterating so you don't need to write code that explicitly knows where each color component is; this is far preferable to having to write a template specialization for every format (which, by the way, means you might as well be writing three separate classes anyway) that duplicates a bunch of common code and differs only by some offsets.


Thank you. A nice solution.

Quote:

(1) As I've hinted at before, templates make it harder for you. You'll need to specialize the class for each format in order to change the type of the data member, which means you're really just writing three classes since specialization is not an inheritance relationship and you must reimplement the entire class for the specialization.


Actually, I'm using traits to do this. But never mind, I'll use your solution and drop the templates.

Share this post


Link to post
Share on other sites
One problem with the solution:

Let's say now I want to change the color of the first pixel to white. How to do that?

const UINT ColorWhite = 0xffffff; //white color for F888 image
mData[0] = ColorWhite; //doesn't work

Share this post


Link to post
Share on other sites
Abstract away the color (don't represent it as an int), or at the very least, abstract away the pixel access (e.g., write a SetPixel method and use the offsets structure I mentioned to figure out where to put the data.

Share this post


Link to post
Share on other sites
Quote:
Original post by jpetrie
Abstract away the color (don't represent it as an int), or at the very least, abstract away the pixel access (e.g., write a SetPixel method and use the offsets structure I mentioned to figure out where to put the data.


I'm getting confused. Do you mean setting each color component individually? Like:

const UINT RedMask = 0xff0000;
const UINT GreenMask = 0x00ff00;
const UINT BlueMask = 0x0000ff;

void SetPixel(int x, int y, UINT color) {
char *p = mData + (mImageWidth * y + x) * mPixelsize;
*p = (color & RedMask) >> 16;
*(++p) = (color & GreenMask) >> 8;
*(++p) = color & BlueMask;
}

This method doesn't work with F555. Also, there's a performance hit (3 assignments per pixel).

Share this post


Link to post
Share on other sites
Quote:

This method doesn't work with F555.

Correct.
You'd need a different method to set a F888 pixel and a F555 pixel, obviously. There are a number of ways you could abstract that out.

Quote:

Also, there's a performance hit (3 assignments per pixel).

Yes, and? The hit is relatively minor unless you do extensive per-pixel manipulation, which is slow and generally a bad idea. Larger raster operations (drawing a rectangle) should be block copies.

Do you intend this Image class to be used for textures, or as a surface for doing 2D rendering? The design concerns are different.

Textures have a reasonable need to support multiple formats. They don't need rasterization operations (such as pixel setting and getting, or line/circle/rectangle functions), or at least not fast ones.

Rasterization surfaces, for example for a software renderer or for a 2D game or whatever, don't need format options. They should be a fixed format (ideally 32 bits per pixel). They need efficient pixel setting, getting and rasertization operations (such line/circle/rectangle/triangle...these functions should not be implemented in terms of the public "GetPixel" and "SetPixel" methods, though it is acceptable to implement them in terms of a common internal pixel manipulation routine that can make some efficiency assumptions).

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement