Opened 8 years ago

Closed 8 years ago

#5293 closed defect (fixed)

ods driver IODSCellEvaluator missing virtual destructor

Reported by: Kurt Schwehr Owned by: Kurt Schwehr
Priority: normal Milestone: 1.10.2
Component: OGR_SF Version:
Severity: minor Keywords:
Cc:

Description (last modified by Kurt Schwehr)

autotest's ogr_ods.py returns the same results without or with the virtual destructor. Fix by abuchbinder at Google.

Adding this to trunk.

Would also like to add this to the 1.10 branch.

    virtual ~IODSCellEvaluator() {}

Change History (6)

comment:1 Changed 8 years ago by Kurt Schwehr

Description: modified (diff)
Status: newassigned

comment:2 Changed 8 years ago by Even Rouault

For my own education, is the definition of the destructor that does nothing just a best practice or something mandated by the C++ language and if you don't do it, the behaviour is undefined (but apparently works well with the usual compilers) ?

Is this abuchbinder tool something private to Google or more generally available ?

comment:3 Changed 8 years ago by Kurt Schwehr

abuchbinder is Andrew - a Googler who jumped in to help cleanup a small aspect of a GDAL build here.

I'm trying to bring back my rusty C++ skills, but:

"If your class has virtual methods, its destructor should be virtual." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheritance#Inheritance

A better reference would be: Page 62-63, Item 14, Scott Meyers, Effective C++, 2nd ed.

Basically, providing that simple NOP virtual destructor definition makes sure that the parents' destructors are called as expected top from top down. You can often get away with leaving it out, but best not to.

comment:4 in reply to:  3 Changed 8 years ago by Kurt Schwehr

Correction... Adam. Not Andrew

Replying to goatbar:

abuchbinder is Andrew - a Googler who jumped in to help cleanup a small aspect of a GDAL build here.

comment:5 Changed 8 years ago by Even Rouault

You can apply in 1.10 branch too if you want.

comment:6 Changed 8 years ago by Kurt Schwehr

Resolution: fixed
Status: assignedclosed

r26616 on trunk and r26641 on 1.10

rouault, thanks for the go-ahead on 1.10

Note: See TracTickets for help on using tickets.