[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:
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?
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
At 1:18 PM -0800 11/21/05, Robert Ramey wrote:
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
-Wno-non-virtual-dtor. But then one loses that warning when it actually matters.
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.
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.
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.
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?
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
-Wno-non-virtual-dtor. But then one loses that warning when it actually matters.
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.
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
I agree (I use serialization, gcc, and -Wall). If the previous release didn't complain then it should be treated as a regression. Darren
Darren Cook wrote:
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
-Wno-non-virtual-dtor. But then one loses that warning when it actually matters.
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.
No, it's not possible. There were some motion to add warning control pragmas, but I don't think anything was done yet.
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
I agree (I use serialization, gcc, and -Wall). If the previous release didn't complain then it should be treated as a regression.
Yes, -Wall is a pretty common setup. - Volodya
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:
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:
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?
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
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 Nov 22, 2005, at 9:15 PM, Kim Barrett 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?
I would like to support this opinion. We have several libraries and applications based on boost and usually get a large number of user complaints for each warning produced when compiling our codes. Matthias
On Tue, 22 Nov 2005 22:11:33 +0100, Matthias Troyer wrote
On Nov 22, 2005, at 9:15 PM, Kim Barrett wrote:
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?
I would like to support this opinion. We have several libraries and applications based on boost and usually get a large number of user complaints for each warning produced when compiling our codes.
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:
On Tue, 22 Nov 2005 22:11:33 +0100, Matthias Troyer wrote
On Nov 22, 2005, at 9:15 PM, Kim Barrett wrote:
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?
I would like to support this opinion. We have several libraries and applications based on boost and usually get a large number of user complaints for each warning produced when compiling our codes.
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 _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 11/22/05, Robert Ramey <ramey@rrsd.com> wrote:
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.
Not to play the wet blanket here, but this would introduce a link-time incompatibility between 1.33.0 and 1.33.1 wouldn't it? -- Caleb Epstein caleb dot epstein at gmail dot com
Caleb Epstein wrote:
On 11/22/05, Robert Ramey <ramey@rrsd.com> wrote:
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.
Not to play the wet blanket here, but this would introduce a link-time incompatibility between 1.33.0 and 1.33.1 wouldn't it?
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
On 11/23/05, Rene Rivera <grafik.list@redshift-software.com> wrote:
Caleb Epstein wrote:
Not to play the wet blanket here, but this would introduce a link-time incompatibility between 1.33.0 and 1.33.1 wouldn't it?
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?
I thought that was one of the implications of it being a patch-level release. That it was ABI compatible. -- Caleb Epstein caleb dot epstein at gmail dot com
Caleb Epstein wrote:
On 11/23/05, Rene Rivera <grafik.list@redshift-software.com> wrote:
Caleb Epstein wrote:
Not to play the wet blanket here, but this would introduce a link-time incompatibility between 1.33.0 and 1.33.1 wouldn't it?
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?
I thought that was one of the implications of it being a patch-level release. That it was ABI compatible.
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