Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ofRectangle is not PoD #1972

Open
SoylentGraham opened this Issue · 12 comments

6 participants

@SoylentGraham

I need a PoD ofRectangle, (for speed) and currently there is a virtual destructor so there's a vtable. Is this neccessary?
I've not seen anything that extends it AND uses any virtuals... (and nothing is declared virtual)
Other of PoD types like ofColour, ofVecXX, even ofPixels don't have virtual destructors...

@bakercp
Collaborator

I often extend ofRectangle in my code, but basically I'm just extending and inheriting and not overriding. So, this should probably be removed.

Out of curiosity what kind of speed hit are you seeing? Do you have some test code for comparison? Just curious what kind of difference it makes.

@SoylentGraham

I don't really have any test code, the rectangle is inside another PoD object of which I'm copying 2,000-8,000 [1] elements from one array(My own array type) to another and across threads and heaps with a lock, so I need to reduce the time as much as possible. A memcpy solves that, but I can't do it at the moment as the vtable is trashed.

In my case it makes a big difference as having my super class as a PoD reduces allocation and copying time. I guess 99.99% cases it wouldn't matter, but then those cases probably don't need any virtuals either :)

[1] It won't be this bad in future, but right now it's a necessary evil in my app

@elliotwoods
Collaborator

PoD = payable on death?

@SoylentGraham

Plain old data. Essentially types you can safely memcpy.
No virtuals and contents that don't require constructors, destructors.

@elliotwoods
Collaborator

that makes more sense! :)

@ofTheo
Owner

I actually just ran into this issue.
I needed to memcpy 40,000 rects to a binary buffer and then back again - the vtable made that impossible without creating a separate type for just the data and then converting back and forth to that.

I think it would be safe to remove the virtual destructor.
none of the other functions are virtual so it is sort of useless ( as I understand ) as it currently is.
curious what @arturoc and @memo thinks - ( I know memo is a big fan of virtual destructors :)

@SoylentGraham

here's my quick work around. (I really need to sort my b0rked branches so I can start making pull requests..)

class ofPodRectangle
{
public:
    ofPodRectangle(const ofRectangle& Rect=ofRectangle()) :
        x   ( Rect.x ),
        y   ( Rect.y ),
        w   ( Rect.width ),
        h   ( Rect.height )
    {
    }

    operator    ofRectangle() const {   return ofRectangle( x, y, w, h );   }

public:
    float   x,y,w,h;
};

ofRectangle a( 10, 10, 100, 100 );
ofPodRectangle b = a;
a = b;
@ofTheo
Owner

ha I did almost the same thing :)
thanks for sharing.

@memo

It's not that I'm a fan of virtual destructors per se, but there are guidelines as to when to use them or not :)
http://stackoverflow.com/questions/461203/when-to-use-virtual-destructors
https://www.google.co.uk/search?q=when+to+use+virtual+destructors

In short if anything is going to extend ofRectangle, and you wish to delete these objects via a pointer to ofRectangle, then ofRectangle has to have a virtual destructor, otherwise the extending class's destructor won't get called and you can get memory leaks if the extending class's destructor was supposed to do memory cleanup.

I don't know if there are any classes in OF which extend ofRectangle? I should imagine in this case it should be ok to lose the virtual destructor.

It is also worth pointing out that even if it becomes a POD, it is not what one would expect from a simple rectangle: i.e. 4 values (x, y, width, height). But actually an ofPoint (which is 3 floats), a width and a height (2 more floats), and then 2 references (for x and y). So actually 7 values.

I"ve never needed to deal with massive arrays of rectangles, and thus never ran into performance issues regarding this. But I should imagine that if that is a bottleneck one would want to use a compact POD which only has the 4 required values float x, y, w, h. (or ofVec2f pos, ofVec2f size);

@arturoc
Owner

i think it's ok to not have virtual destructors if there's no other virtual functions. the destructors of a class inherinting from ofRectangle will only fail if you try to destroy them from an ofRectangle pointer:

class RectShape: public ofRectangle{
   ....
}

RectShape * shape = new RectShape;
...
delete shape;  // this will work as expected


void someFunction(ofRectangle * rectangle){
    ...
    delete rectangle;  // this will be problematic
}

that last case is pretty weird since ofRectangle doesn't have virtual functions using a class that inherits from it polymorphically as an ofRectangle doesn't make sense. if you have a collection of objects that inherit from ofRectangle there's probably some other common base to all of them

also perhaps we should remove the references and the ofPoint in ofRectangle, the position can be a function that creates an ofPoint

@SoylentGraham

Nothing in the core extends it, but ofxUIRectangle does, (in ofxUI) though it has no destructor.

If you're explicitly extending a PoD (eg. for multiple inheritance) I wouldn't expect to be deleting it virtually (as arturo exampled), it's a bit of a wierd case (for dumb shapes at least) if your derivative needs to be destructed properly but you're deleting/storing it as a PoD pointer type...

As for the x/y being a vec3f point... I didn't actually notice that. Do we ever use the Z component in the rectangle? Having the z makes it a little odd IMO (w/h is on which axis?) ofBox is for 3D shapes...

All just my opinion of course, but ofColour, ofVecXX are PoD's without virtuals.

@bakercp
Collaborator

Regarding ofPoint/vec3f -- yes it is odd. Here's a related discussion that you might want to weigh in on.

#1821

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.