[serialization] non-virtual destructors in 1.33.1
Between 1.33 and 1.33.1_beta, several classes in the serialization library were changed from having virtual destructors to having non-virtual destructors. An example is basic_oserializer. Many of these classes have virtual functions, with the result that "g++ -Wall" now complains about them ("has virtual functions but non-virtual destructor"), so that I now get tons of warnings when compiling code that uses the serialization library. My guess would be that none of these warnings are actually interesting, but that belief doesn't really help all that much. Was there a good reason for this change? Could it be reverted? If not, is there some other thing that could be done to eliminate these warnings? For 1.33.1 release?
You might consider adding a switch to the gcc compiler switches used. This is what I did to make it pass the tests without complaint. Offhand I've forgotten the exact switch but I think you'll find it in serialization.jam in libs/serialization/build The warnings can be ignored in this case. The destrctor is "protected" so that attempts to delete through a base class pointer will be detected as errors at compile time. This also eliminates a useless vtable entry. This was introduced to be sure that derived classes in DLLS wern't being deleted through the library. However, these warnings are annoying. I'm considering adding "virtual" back in to the gcc compiles just to avoid these annoying warnings - but this would be in a future release in any case. Robert Ramey Kim Barrett wrote:
At 1:18 PM -0800 11/21/05, Robert Ramey wrote:
-Wno-non-virtual-dtor. But then one loses that warning when it actually matters.
OK, so these warnings really are uninteresting. Unfortunately, the heuristic (virtual functions + non-virtual destructor => potentially bad) is inadequate for detecting that. And I'm not sure that modifying that heuristic to only apply when the destructor is public would actually detect some of the real errors this heuristic warning is trying to prevent, so I doubt I would get any traction by filing a bug report against gcc.
Well, what's a beta period for, if not to shake out things that one really doesn't want to release and live with for a while. I guess this is really a request to the release manager to consider allowing this change to be made for 1.33.1. Rationale: common warning settings on very popular tool chain will generate potentially huge numbers of spurious warnings for users of the serialization library without this change. Is there a mechanism for directly bringing something like this to the attention of the release manager (is that Doug?)? Direct email?
Does GCC support a way to switch warnings on and off from within the code? IIRC Visual C++ has something like that. If so the serialization headers could just suppress that warning.
I agree (I use serialization, gcc, and -Wall). If the previous release didn't complain then it should be treated as a regression. Darren
If this such a huge problem for you, why don't you just modify your local copy and be done with it? I wouldn't think that one would want to delay an overdue release to avoid a bogus warning. This a gcc problem in any case. There is a limit to how much stuff we should have to work around. Robert Ramey Robert Ramey wrote:
At 8:55 PM -0800 11/21/05, Robert Ramey wrote:
If this such a huge problem for you, why don't you just modify your local copy and be done with it?
I think you are missing my point. I've already addressed this locally (by adding the warning suppression option to our compiler configuration, and then filing a bug report against that configuration to remind us to change it back when the underlying problem is addressed). However, do you (and the rest of the boost community) really want to release this in it's current state? Should every development group using gcc and the serialization library have to locally work around this? How many queries about this are there going to be on the boost mailing lists between now and a release that addresses this? And how many potential users of the library are going to try it, get large numbers of warnings, and go elsewhere? If it's a matter of finding time to write the patch, I'll volunteer to do that.
On Tue, 22 Nov 2005 22:11:33 +0100, Matthias Troyer wrote
On Nov 22, 2005, at 9:15 PM, Kim Barrett wrote:
We really need a volunteer to fix the regression system so that developers can actually see warnings -- be aware that when things compile with warnings there is NO indication in the regression system. So rather than just fix the one problem, we need someone to work on the general problem so we can start tracking warnings as an issue before release rather than after... Jeff
FWIW - I saw the warnings on my own system. I use comipler_status_2 on my system which does show boxes with warnings. I decided to leave it alone for the following reasons. a) "Official" boost policy is that there is no requirement for a library to make special accomodation for non-conforming compilers b) Emision of a warning is not an error in any case. c) gcc allows one to supress the warning in the command line. d) I made the destructors non-virtual and placed them in the protected section because I specifically did NOT the object destructor called from anywhere but the most derived class. This was to address some issues where the derived class can be defined in a dynamically loaded DLL while the base class can be defined in the serialization library or DLL. If the derived class DLL is explicilty unloaded while an object defined by it still exists there will be a program crash. This can be hard to avoid as some of the base class instances are static variables destroyed on program exit. e) Doing things the way I did permits any errors in this code to be detected at compile time. Having said that, and (hopefully) the code having been tested, I'm hopeful that making the destructor virtual for gcc won't hurt anything. Doug has asked me to add it in so this warning won't appear on this compiler. So that's what we're going to do. Robert Ramey Jeff Garland wrote:
Caleb Epstein wrote:
Yes (AFAIK), but how is pointing that out being a wet blanket? Or are you implying that 1.33.0 and 1.33.1 should be link compatible? -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - Grafik/jabber.org
Caleb Epstein wrote:
For many other libraries, yes. For C++ it's not as common. For Boost, contrary to what I though for some time, it's not the case. About the only compatibility for patch level releases we can count on is API compatibility. -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - Grafik/jabber.org
participants (8)
-
Caleb Epstein -
Darren Cook -
Jeff Garland -
Kim Barrett -
Matthias Troyer -
Rene Rivera -
Robert Ramey -
Vladimir Prus