Fix -Wnon-virtual-dtor compiler warnings.

Fix -Wnon-virtual-dtor compiler warnings.

This is done by explicitly adding a virtual dtor to interface classes,
since the cost is small given that there are already virtual functions.

The exceptions are for classes that have a Release() or Delete() method,
in which case it is non-virtual and protected to indicate that the virtual
class is never the deletion point.

BUG=
R=brucedawson@chromium.org, thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/b9e0190938f62bf21df078e47190a62ba33ddab2

[Tom Sepez, 2015-01-08 22:17:20]

tsepez@chromium.org changed reviewers:
+ brucedawson@chromium.org, leilei@chromium.org

[Tom Sepez, 2015-01-08 22:17:21]

Lei, Bruce, please review. This may take a careful eye :(.

[brucedawson, 2015-01-08 22:42:37]

Initial evaluation from the compiler: this reduces the number of lines of
warnings (Debug builds) from 26,757 to 1,961, a reduction of 24,796, or 92.7%.
Which is good.

expr $(cat results.txt | grep -v "] " | wc -l) - $(cat results2.txt | grep -v "]
" | wc -l)

[Tom Sepez, 2015-01-08 23:08:09]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org
- leilei@chromium.org

[Tom Sepez, 2015-01-08 23:08:19]

Sorry, wrong Lei.

[Lei Zhang, 2015-01-08 23:18:12]

lgtm

[brucedawson, 2015-01-09 00:20:50]

A few comments to think about.

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_dib.h
File core/include/fxge/fx_dib.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_dib...
core/include/fxge/fx_dib.h:416: virtual ~CFX_ScanlineCompositor();
This one feels dirty because clearly CFX_Object does not have a virtual
destructor, so adding one here doesn't guarantee correctness. Do the protected
destructor trick here instead?

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_ge....
core/include/fxge/fx_ge.h:640: virtual void  Release() = 0;
Are there any binary compatibility issues from reordering these? i.e.; are there
ever any cases where the two sides of the interface are not updated
simultaneously? I assume not, but just checking.

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
File core/src/fpdfapi/fpdf_font/ttgsubtable.cpp (right):

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
core/src/fpdfapi/fpdf_font/ttgsubtable.cpp:449: pGsubTable->Release();
I guess this is one vote in favor of protected destructors in classes with
Release. Good fix.

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:421: ~CFX_GSUBTable() { }
Shouldn't this be dealt with by one of the base classes? I'm guessing that this
is unnecessary or that a protected or virtual destructor in the base class that
introduces Release and/or GetVerticalGlyph would be better.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/fxedit/f...
File fpdfsdk/include/fxedit/fx_edit.h (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/fxedit/f...
fpdfsdk/include/fxedit/fx_edit.h:148: virtual void							Release() = 0;
Same reordering question. Probably fine, but just checking.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/pdfwindo...
File fpdfsdk/include/pdfwindow/PWL_Note.h (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/pdfwindo...
fpdfsdk/include/pdfwindow/PWL_Note.h:30: virtual ~IPWL_NoteNotify() { }
Indentation looks weird. Spaces versus tabs I think. Ditto for the other changes
in this file, and probably elsewhere.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/src/fpdf_sysfont...
File fpdfsdk/src/fpdf_sysfontinfo.cpp (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/src/fpdf_sysfont...
fpdfsdk/src/fpdf_sysfontinfo.cpp:80: ~CSysFontInfo_Ext() { }
It feels like this should be dealt with by the class that introduces the virtual
functions not in the derived class.

[Tom Sepez, 2015-01-09 01:16:53]

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_dib.h
File core/include/fxge/fx_dib.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_dib...
core/include/fxge/fx_dib.h:416: virtual ~CFX_ScanlineCompositor();
On 2015/01/09 00:20:49, brucedawson wrote:
> This one feels dirty because clearly CFX_Object does not have a virtual
> destructor, so adding one here doesn't guarantee correctness. Do the protected
> destructor trick here instead?
I don't think this one ended up needing to be modified.  Will remove.

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/include/fxge/fx_ge....
core/include/fxge/fx_ge.h:640: virtual void  Release() = 0;
There's one cast but its not suspicious - in fx_win32_print.cpp:298 - looks
unnecessary.  Otherwise, I didn't see any casts to ifx_psoutput, and just the
one derived class.

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/810883005/diff/20001/core/src/fpdfapi/fpdf_fo...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:421: ~CFX_GSUBTable() { }
On 2015/01/09 00:20:49, brucedawson wrote:
> Shouldn't this be dealt with by one of the base classes? I'm guessing that
this
> is unnecessary or that a protected or virtual destructor in the base class
that
> introduces Release and/or GetVerticalGlyph would be better.

removed, the protected destructor is present in IFX_GSUBTable.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/fxedit/f...
File fpdfsdk/include/fxedit/fx_edit.h (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/fxedit/f...
fpdfsdk/include/fxedit/fx_edit.h:148: virtual void							Release() = 0;
On 2015/01/09 00:20:49, brucedawson wrote:
> Same reordering question. Probably fine, but just checking.

Didn't see any casts between IFX_Edit_UndoItem and CFX_Edit_UndoItem,
CFX_Edit_GroupUndoItem.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/pdfwindo...
File fpdfsdk/include/pdfwindow/PWL_Note.h (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/include/pdfwindo...
fpdfsdk/include/pdfwindow/PWL_Note.h:30: virtual ~IPWL_NoteNotify() { }
On 2015/01/09 00:20:49, brucedawson wrote:
> Indentation looks weird. Spaces versus tabs I think. Ditto for the other
changes
> in this file, and probably elsewhere.

Tabs are the bane of my existence. Replaced with single tab indent.

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/src/fpdf_sysfont...
File fpdfsdk/src/fpdf_sysfontinfo.cpp (right):

https://codereview.chromium.org/810883005/diff/20001/fpdfsdk/src/fpdf_sysfont...
fpdfsdk/src/fpdf_sysfontinfo.cpp:80: ~CSysFontInfo_Ext() { }
On 2015/01/09 00:20:49, brucedawson wrote:
> It feels like this should be dealt with by the class that introduces the
virtual
> functions not in the derived class.

Done.

[Tom Sepez, 2015-01-09 01:21:42]

Warnings back now that I pulled some of the declarations we thought were not
required.

core/src/fpdfapi/fpdf_font/ttgsubtable.h:409:7: warning: ‘class CFX_GSUBTable’
has virtual functions and accessible non-virtual dest
ructor [-Wnon-virtual-dtor]
core/src/fpdfapi/fpdf_font/ttgsubtable.h:409:7: warning: ‘class CFX_GSUBTable’
has virtual functions and accessible non-virtual dest
ructor [-Wnon-virtual-dtor]
fpdfsdk/src/fpdf_sysfontinfo.cpp:10:7: warning: ‘class CSysFontInfo_Ext’ has
virtual functions and accessible non-virtual destructor
 [-Wnon-virtual-dtor]
t

[Tom Sepez, 2015-01-09 17:40:49]

Bruce, are we good to go with ps#4?

[brucedawson, 2015-01-09 18:30:08]

lgtm

[Tom Sepez, 2015-01-09 18:46:55]

Committed patchset #4 (id:60001) manually as
b9e0190938f62bf21df078e47190a62ba33ddab2 (tree was closed).