[1.34.0beta] many, many warnings... :(
Hei, I'm trying to build the project I'm working on with boost 1.34.0-beta. We have used 1.33.1, and 1.32 before that. I cannot recall having seen so many warnings being spit at me than now (of course, we have started to use a lot more boost libraries in the last two years, and I have been using the Debian packages for 1.33.1 which apparently had a slew of those warnings fixed). I see not only warnings for esoteric levels like when using a C-style cast, but also whenever BOOST_WORKAROUND is used, which apparently expands to something like #if __MACRO__ rel version which prompts gcc to warn about __MACRO__ not being defined. This is purely a quality of implementation issue, I know, and I have seen people sending patches for these things (incidentally, Debian people :), but I want to put in my voice from the trench that I'm disappointed that less and less of boost compiles w/o warnings. I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>. And: Debianers, keep up the good work! Thanks, Marc -- Marc Mutz - marc@kdab.com, mutz@kde.org - Klarälvdalens Datakonsult AB Platform-independent software solutions - www.kdab.com info@kdab.com
Hi, Marc Mutz wrote:
This is purely a quality of implementation issue, I know, and I have seen people sending patches for these things (incidentally, Debian people :), but I want to put in my voice from the trench that I'm disappointed that less and less of boost compiles w/o warnings. I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. I strongly second that point of view. Starting with 1_33_0 we had to made our boost branches here just to get rid of some of the most annoying warnings. E.g. gcc warns about local variable names hiding other variables or function names (e.g. in date_time\posix_time\posix_time_config.hpp:85, 'ticks'). These sorts of warnings are easily avoidable.
Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>. Or do it the hard way: treat warnings as errors.
Best regards Olaf Krzikalla
Olaf Krzikalla <krzikalla <at> gmx.net> writes: [...]
I strongly second that point of view. Starting with 1_33_0 we had to made our boost branches here just to get rid of some of the most annoying warnings. E.g. gcc warns about local variable names hiding other variables or function names (e.g. in date_time\posix_time\posix_time_config.hpp:85, 'ticks'). These sorts of warnings are easily avoidable.
Did you post your patches to the SourceForge tracker? Cheers, Nicola Musatti
Marc Mutz wrote:
I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>.
The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code. - Volodya
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, May 02, 2007 8:07 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Marc Mutz wrote:
I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>.
The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code.
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff. Usually the warnings I've seen in Boost are pretty easy to fix but its understandable that that puts yet another load on the developers. The "problem" with patches is that they take forever to get accepted and/or commented on as they are lots of little loads and resources are limited. Warning-fixing patches are understandably lower priority than segfault-fixing ones. Maybe a catch-all-warning-fixes bug is in order if we can get some sort of resource with commit privilege to look over it? The prerequisite is that the patches should ONLY quiet warnings. If we do this, and then at some point accept this catch-all set of patches, then we can turn on warnings as errors once the patches have been merged. I've had to do this before, and this works as far as setting you up for the future. Sohail
Sohail Somani wrote:
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, May 02, 2007 8:07 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Marc Mutz wrote:
I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>.
The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code.
If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
and if bjam has an option to "keep going" as much as possible,
It has, and it's the default. - Volodya
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus
The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code.
If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
I don't see why it wouldn't be (after the warnings have been quieted.) Can anyone see why? I know atleast the serialization library wants to ignore the "no virtual destructor" warning, but explicitly ignoring the warnings are par for the course providing you've investigated it (I think Robert R. has.) Thanks, Sohail
Vladimir Prus wrote:
Sohail Somani wrote:
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, May 02, 2007 8:07 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Marc Mutz wrote:
I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>. The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code. If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
The problem is that some warnings can't be avoided, at least not if you try to be portable. (Example: As discussed in a different thread: putting in a return statement to satisfy some compilers may trigger a 'unreachable code' warning on others.) Regards, Stefan -- ...ich hab' noch einen Koffer in Berlin...
Stefan Seefeld wrote:
<pick your favorite color>. The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code. If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
The problem is that some warnings can't be avoided, at least not if you try to be portable. (Example: As discussed in a different thread: putting in a return statement to satisfy some compilers may trigger a 'unreachable code' warning on others.)
I think BOOST_AVOID_WARNING_XXX macro can be used to suppress a warning in compiler-specific way. We probably can use warnings-as-errors only for gcc and msvc, to reduce portability burden. - Volodya
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, May 02, 2007 9:40 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Stefan Seefeld wrote:
<pick your favorite color>. The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code. If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
The problem is that some warnings can't be avoided, at least not if you try to be portable. (Example: As discussed in a different thread: putting in a return statement to satisfy some compilers may trigger a 'unreachable code' warning on others.)
I think BOOST_AVOID_WARNING_XXX macro can be used to suppress a warning in compiler-specific way.
We probably can use warnings-as-errors only for gcc and msvc, to reduce portability burden.
That sounds an awesome idea. Shall I file a bug? :) Sohail
Vladimir Prus wrote:
We probably can use warnings-as-errors only for gcc and msvc, to reduce portability burden.
cxx (Tru64, VMS) has -msg_error option and aCC (HP-UX) has +We and +Weargs options. They all increase message(s) severity to error. When running boost tests on HP-UX/aCC6, I suppress 53 (!) warnings (via CXXOPTS env. variable). Thanks, Boris ----- Original Message ----- From: "Vladimir Prus" <ghost@cs.msu.su> To: <boost@lists.boost.org> Sent: Wednesday, May 02, 2007 12:39 PM Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Stefan Seefeld wrote:
<pick your favorite color>. The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code. If you treat warnings as errors
Fine we me, but not necessary fine with everybody ;-). And I suspect Boost.Build's warnings-as-errors=on work only with few compilers. But few is better than none.
The problem is that some warnings can't be avoided, at least not if you try to be portable. (Example: As discussed in a different thread: putting in a return statement to satisfy some compilers may trigger a 'unreachable code' warning on others.)
I think BOOST_AVOID_WARNING_XXX macro can be used to suppress a warning in compiler-specific way.
We probably can use warnings-as-errors only for gcc and msvc, to reduce portability burden.
- Volodya
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
----- Mensaje original ----- De: Stefan Seefeld <seefeld@sympatico.ca> Fecha: Miércoles, Mayo 2, 2007 6:37 pm Asunto: Re: [boost] [1.34.0beta] many, many warnings... :( Para: boost@lists.boost.org [...]
(Example: As discussed in a different thread: putting in a return statement to satisfy some compilers may trigger a 'unreachable code' warning on others.)
This particular prolem is already addressed by Boost.Config macro BOOST_UNREACHABLE_RETURN: http://www.boost.org/libs/config/config.htm Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
on Wed May 02 2007, "Sohail Somani" <s.somani-AT-fincad.com> wrote:
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Vladimir Prus Sent: Wednesday, May 02, 2007 8:07 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Marc Mutz wrote:
I for one think that this is a serious issue, and I encourage you to accept such patches (not for 1.34.0, obviously, but 1.34.1) and make it a release goal for 1.35 (or 1.34.1) to reduce the number of warnings to a decent level. Maybe change the regression tests to highlight any kind of compiler diagnostics in <pick your favorite color>.
The problem is that the current regression reporting tools don't count warnings (previous version use to), so there's nothing nagging developers about warnings introduced in their code.
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff.
It would take a lot to convince me to require that Boost builds with all warnings enabled and treated as errors. There are just too many "nuisance" warnings out there, and since GCC gives us no way to explicitly suppress warnings in code, we'd have to write convoluted (and sometimes even inefficient) code just to silence them. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com Don't Miss BoostCon 2007! ==> http://www.boostcon.com
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of David Abrahams Sent: Wednesday, May 02, 2007 10:48 AM
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff.
It would take a lot to convince me to require that Boost builds with all warnings enabled and treated as errors. There are just too many "nuisance" warnings out there, and since GCC gives us no way to explicitly suppress warnings in code, we'd have to write convoluted (and sometimes even inefficient) code just to silence them.
But most (not all, for sure) warnings can be disabled by -fno-some-feature. The only places where you might have to change the code would be in the cases where the warning cannot be disabled. Anyway, isn't it enough to convince you to think about it that someone has taken a branch locally just to silence warnings? I've done something similar by having a set of patches that I applied to pristine sources. I can't imagine we're the only ones. Having "disable this warning" flags in the build file may make it apply to more files than is really necessary, but I believe bjam does handle per-file application of flags quite nicely. Though I don't use it so I might be wrong. Header-only libraries however... Sohail
On Wed, 2 May 2007 10:59:56 -0700 "Sohail Somani" <s.somani@fincad.com> wrote:
But most (not all, for sure) warnings can be disabled by -fno-some-feature. The only places where you might have to change the code would be in the cases where the warning cannot be disabled.
That's the problem. You don't want to disable the warning entirely. That's worse than ignoring them. You want to disable a warning at the point it is a warning. This is a MAJOR drawback of gcc...
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Jody Hagins Sent: Wednesday, May 02, 2007 11:59 AM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
On Wed, 2 May 2007 10:59:56 -0700 "Sohail Somani" <s.somani@fincad.com> wrote:
But most (not all, for sure) warnings can be disabled by -fno-some-feature. The only places where you might have to change the code would be in the cases where the warning cannot be disabled.
That's the problem. You don't want to disable the warning entirely. That's worse than ignoring them. You want to disable a warning at the point it is a warning.
This is a MAJOR drawback of gcc...
So then there are two options given the two major toolsets: * Change the code to quiet it (along the lines of BOOST_WARNING_XXX macros) or fix it if it is a real warning * Add a warning flag when compiling the specific file If the first option isn't feasible for a given warning, then the second one may apply as a last resort. And there is still the option of ignoring the warnings, but Boost is a harder sell if it cannot compile without warnings. That's been my experience. Sohail
On Wednesday 02 May 2007 19:04, Sohail Somani wrote:
* Add a warning flag when compiling the specific file
Just to make this clear: I'm not concerned about warnings when _compiling_ boost, I'm concerned about warnings that including boost headers spit at me in my _own_ projects. E.g. I get a warning about a C-style cast whenever using BOOST_STATIC_ASSERT. And I don't even want to start about the pages full of warnings a simple include of 1.34's boost/function.hpp generates, dt BOOST_WORKAROUND. Thanks, Marc -- Marc Mutz -- marc@klaralvdalens-datakonsult.se, mutz@kde.org Klarälvdalens Datakonsult AB, Platform-independent software solutions
There is an alternative approach. These warnings could be documented on the Boost website so that users can look them up for themselves and be advised what action, if any, they need take. (In most cases, this would be "this warning can be safely ignored - no action on the programmer's part is required"). There would be a section for each compiler for which Boost is available. This would require quite a lot of work, of course, but it would help put users' minds at rest and would not require any changes to be made to the code. Apart from the effort involved, the only other drawback I see is warnings might not come to light until Boost libraries are actually used, or might depend on the way the library is used. Perhaps, in that case, there could be an option for users to make requests about the severity of warnings as and when they encountered them, and for a record of them to be built up on the site in this way. No doubt a poll of the posters to this list ("What warnings in Boost code does your compiler give when you compile code using Boost libraries?") would go some way to getting it started. I don't know how practical this is, but it's an idea that might be worth considering.
On Thu, May 03, 2007 at 08:05:39AM +0000, Marc Mutz wrote:
On Wednesday 02 May 2007 19:04, Sohail Somani wrote:
* Add a warning flag when compiling the specific file
Just to make this clear: I'm not concerned about warnings when _compiling_ boost, I'm concerned about warnings that including boost headers spit at me in my _own_ projects.
i agree, these are those to be managed at best. think at linux distributions packaging... regards domenico -----[ Domenico Andreoli, aka cavok --[ http://www.dandreoli.com/gpgkey.asc ---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
on Wed May 02 2007, Jody Hagins <jody-boost-011304-AT-atdesk.com> wrote:
On Wed, 2 May 2007 10:59:56 -0700 "Sohail Somani" <s.somani@fincad.com> wrote:
But most (not all, for sure) warnings can be disabled by -fno-some-feature. The only places where you might have to change the code would be in the cases where the warning cannot be disabled.
That's the problem. You don't want to disable the warning entirely. That's worse than ignoring them. You want to disable a warning at the point it is a warning.
I.e. in the code, not on the compiler command line.
This is a MAJOR drawback of gcc...
Yup. -- Dave Abrahams Boost Consulting www.boost-consulting.com Don't Miss BoostCon 2007! ==> http://www.boostcon.com
On 5/2/07, David Abrahams <dave@boost-consulting.com> wrote:...
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff.
It would take a lot to convince me to require that Boost builds with all warnings enabled and treated as errors. There are just too many "nuisance" warnings out there, and since GCC gives us no way to explicitly suppress warnings in code, we'd have to write convoluted (and sometimes even inefficient) code just to silence them.
This is certainly the case with the MS compiler, who even at lower warning levels tends to make spurious comments about your code (not really warnings at all). However with gcc (and possibly other compilers), building w/ -Wall -Werror is tenable, and is usually the Right Thing. We have done this on the last several (extremely large) projects I have worked on. Jon
on Wed May 02 2007, "Jonathan Franklin" <franklin.jonathan-AT-gmail.com> wrote:
On 5/2/07, David Abrahams <dave@boost-consulting.com> wrote:...
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff.
It would take a lot to convince me to require that Boost builds with all warnings enabled and treated as errors. There are just too many "nuisance" warnings out there, and since GCC gives us no way to explicitly suppress warnings in code, we'd have to write convoluted (and sometimes even inefficient) code just to silence them.
This is certainly the case with the MS compiler, who even at lower warning levels tends to make spurious comments about your code (not really warnings at all).
At least it gives me the #pragmas I need to silence them.
However with gcc (and possibly other compilers), building w/ -Wall -Werror is tenable, and is usually the Right Thing. We have done this on the last several (extremely large) projects I have worked on.
In my experience it's the Right Thing for certain common coding styles, but completely wrong for others. For example, GCC has a warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning. -- Dave Abrahams Boost Consulting www.boost-consulting.com Don't Miss BoostCon 2007! ==> http://www.boostcon.com
On 5/4/07, David Abrahams <dave@boost-consulting.com> wrote:
on Wed May 02 2007, "Jonathan Franklin" <franklin.jonathan-AT-gmail.com> wrote: ...
This is certainly the case with the MS compiler, who even at lower warning
levels tends to make spurious comments about your code (not really warnings at all).
At least it gives me the #pragmas I need to silence them.
Indeed. It's a shame that gcc doesn't provide a good 'prama disable'.
However with gcc (and possibly other compilers), building w/ -Wall
-Werror is tenable, and is usually the Right Thing. We have done this on the last several (extremely large) projects I have worked on.
In my experience it's the Right Thing for certain common coding styles, but completely wrong for others.
Hence 'usually'. My coding style apparently falls amongst the common. For example, GCC has a
warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning.
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic. Jon
Jonathan Franklin wrote: :: On 5/4/07, David Abrahams <dave@boost-consulting.com> wrote: ::: ::: ::: For example, GCC has a ::: warning about a derived class whose base doesn't have a virtual ::: dtor. It's actually *impossible* (not just inefficient or ::: convoluted) to implement is_polymorphic without generating that ::: warning. :: :: :: Interesting. I'm obviously flaunting my ignorance, but I didn't :: realize :: that inheriting from a class sans virtual dtor was ever a Good :: Thing. I'll have to read up on the issues WRT is_polymorphic. :: Here's an example from the standard library: template<class _Ty> struct plus : public binary_function<_Ty, _Ty, _Ty> { // functor for operator+ _Ty operator()(const _Ty& _Left, const _Ty& _Right) const { // apply operator+ to operands return (_Left + _Right); } }; Neither plus nor binary_function has any virtual destructor, but they are still useful. You will not use them polymorphically though. Bo Persson
Jonathan Franklin wrote:
For example, GCC has a
warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning.
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic.
I find inheriting without virtual functions quite useful. The main reason for the virtual destructor rule goes back to Scott Meyers Effective C++. He points out that if you have Base <-- Derived and you call delete Base* the destructor of Derived is not called. In general, it's hard to ensure your derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it. As a practical example, my super string 'experiment' uses this technique for interface compatibility with std::string. In one incarnation super_string inherits from std::string, which doesn't have a virtual destructor. Since super_string only adds functions to the type, has no state, and a trivial destructor I don't care that it's not called. So, I have an extended type with functions useful to me, it's backward compatible with the existing type, and no virtual table/destructor is needed. http://www.crystalclearsoftware.com/libraries/super_string/index.html Jeff
----- Mensaje original ----- De: Jeff Garland <jeff@crystalclearsoftware.com> Fecha: Sábado, Mayo 5, 2007 6:10 pm Asunto: Re: [boost] [1.34.0beta] many, many warnings... :( Para: boost@lists.boost.org
Jonathan Franklin wrote:
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic.
I find inheriting without virtual functions quite useful. The main reason for the virtual destructor rule goes back to Scott Meyers Effective C++. He points out that if you have Base <-- Derived and you call delete Base* the destructor of Derived is not called. In general, it's hard to ensure your derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it.
I agree and disagree with your previous exposition :) I think deriving without virtual constructors is fine in those contexts when the user is not expected to delete passed objects, which are by far the most usual. Almost every function of the form void foo([const] T& t) won't certainly try to delete t, so it's fine to pass an use here an object of some class derived from T (like in your super_string case.) IMHO the contexts in which objects are passed around without ownership transfer are overwhelmingly more numerous than those where transfer is done, so I think this "don't derive without virtual dtor" is somewhat overrated. There's the special case when the base is abstract or has some virtual function: here it is more likely that the class was designed with ownership transer via pointers in mind, and the rule would be more sensible. But then again, a great many classes don't have any virtual member function, much more so in the template-dominated C++ of our days. What I don't agree with you in is your claim that "if Derived happens to have a trivial/empty destructor then, really there's no reason to call it." If my memory serves me well, deleting a Derived object through its Base non-virtual dtor is illegal according to the standard, regardless of whether D's dtor is trivial or not. You're merely being lucky with the actual behavior exhibited by compilers. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
JOAQUIN LOPEZ MU?Z wrote:
I find inheriting without virtual functions quite useful. The main reason for the virtual destructor rule goes back to Scott Meyers Effective C++. He points out that if you have Base <-- Derived and you call delete Base* the destructor of Derived is not called. In general, it's hard to ensure your derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it.
I agree and disagree with your previous exposition :)
That's usual ;-)
I think deriving without virtual constructors is fine in those contexts when the user is not expected to delete passed objects, which are by far the most usual. Almost every function of the form
void foo([const] T& t)
won't certainly try to delete t, so it's fine to pass an use here an object of some class derived from T (like in your super_string case.)
Sure, but in the general case you could easily create an interface like this: void foo(T* t) hence Scott's design rule.
...snip...
What I don't agree with you in is your claim that "if Derived happens to have a trivial/empty destructor then, really there's no reason to call it." If my memory serves me well, deleting a Derived object through its Base non-virtual dtor is illegal according to the standard, regardless of whether D's dtor is trivial or not. You're merely being lucky with the actual behavior exhibited by compilers.
By the standard I believe it is technically undefined behavior. But as a practical matter it's defined exactly the same on all compilers -- call the base class destructor. Jeff
Jeff Garland wrote:
By the standard I believe it is technically undefined behavior. But as a practical matter it's defined exactly the same on all compilers -- call the base class destructor.
The relevant quote is in 5.3.5/3: "In the first alternative (delete object ), if the static type of the operand is different from its dynamic type, the static type shall be a base class of the operand’s dynamic type and the static type shall have a virtual destructor or the behavior is undefined." I really don't think that a Boost library should intentionally contain undefined behaviour, even if the actual behaviour is very consistent between implementations. Sebastian Redl
On 5/5/07, Jeff Garland <jeff@crystalclearsoftware.com> wrote:
Jonathan Franklin wrote:
For example, GCC has a
warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning.
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic.
Sorry, I slightly misspoke. What I meant to type was that I didn't realize that inheriting from a class with virtual methods sans virtual dtor was ever a Good Thing. AFAICT gcc will only gripe if the base class defines virtual methods but no virtual dtor. Perhaps this isn't the case with older versions of the compiler. I still need to read up on the issues WRT is_polymorphic, since they obviously exist given David's previous statements. I find inheriting without virtual functions quite useful. I agree wholeheartedly. In general, it's hard to ensure your
derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it.
True. The first case *usually* (but not always) trumps the second, for the reason that you mentioned, and additionally, just because when you wrote the derived class with an empty/trivial dtor doesn't mean that someone else won't add something non-trivial later and forget to add 'virtual' to the base class dtor. Jon
on Sat May 05 2007, "Jonathan Franklin" <franklin.jonathan-AT-gmail.com> wrote:
AFAICT gcc will only gripe if the base class defines virtual methods but no virtual dtor. Perhaps this isn't the case with older versions of the compiler.
I still need to read up on the issues WRT is_polymorphic, since they obviously exist given David's previous statements.
Oh, maybe not. I don't remember the precise issue. However, it's easy to imagine that that particular warning could have been defined differently. My point is that most warnings aren't hard errors for a reason: there are legitimate use cases for the code being warned about. -- Dave Abrahams Boost Consulting www.boost-consulting.com Don't Miss BoostCon 2007! ==> http://www.boostcon.com
David Abrahams wrote:
on Sat May 05 2007, "Jonathan Franklin" <franklin.jonathan-AT-gmail.com> wrote:
AFAICT gcc will only gripe if the base class defines virtual methods but no virtual dtor. Perhaps this isn't the case with older versions of the compiler.
I still need to read up on the issues WRT is_polymorphic, since they obviously exist given David's previous statements.
Oh, maybe not. I don't remember the precise issue. However, it's easy to imagine that that particular warning could have been defined differently. My point is that most warnings aren't hard errors for a reason: there are legitimate use cases for the code being warned about.
As a practical matter, if your code has 100 false warnings, there's no chance you'll notice 101-th warning that's result of real bug. Therefore, you either eliminate all warnings, enable -Werror and investigate all new warnings that occur, or you don't use warnings at all. I prefer -Werror, and therefore if Boost headers produce zillion of warnings, that's rather bad. Now, it might be impractical to expect anybody to spend day workarounding an obscure warning on a compiler nobody uses, but all warning patches I saw posted are rather simple, so maybe there's no hard-to-workaround warnings at all. - Volodya
Vladimir Prus wrote:
David Abrahams wrote:
on Sat May 05 2007, "Jonathan Franklin" <franklin.jonathan-AT-gmail.com> wrote:
AFAICT gcc will only gripe if the base class defines virtual methods but no virtual dtor. Perhaps this isn't the case with older versions of the compiler.
I still need to read up on the issues WRT is_polymorphic, since they obviously exist given David's previous statements. Oh, maybe not. I don't remember the precise issue. However, it's easy to imagine that that particular warning could have been defined differently. My point is that most warnings aren't hard errors for a reason: there are legitimate use cases for the code being warned about.
As a practical matter, if your code has 100 false warnings, there's no chance you'll notice 101-th warning that's result of real bug. Therefore, you either eliminate all warnings, enable -Werror and investigate all new warnings that occur, or you don't use warnings at all. I prefer -Werror, and therefore if Boost headers produce zillion of warnings, that's rather bad.
Now, it might be impractical to expect anybody to spend day workarounding an obscure warning on a compiler nobody uses, but all warning patches I saw posted are rather simple, so maybe there's no hard-to-workaround warnings at all.
I'm inclined to agree. While eliminating all warnings from all compilers isn't realistic, we should have procedures in place to detect and eliminate warnings from common compilers where the fix doesn't have undesirable consequences. In effect, Boost is beginning a period of reengineering for some of our core procedures. The first task is the move to Subversion. The second task is the new release management procedure (and that probably implies at least some reengineering of how we do testing.) It looks like our patch management needs an overhaul too. I don't know what that might look like, but once the new release procedure has firmed maybe we can look at how to better integrate patch management into our development process. --Beman
on Sat May 05 2007, Jeff Garland <jeff-AT-crystalclearsoftware.com> wrote:
Jonathan Franklin wrote:
For example, GCC has a
warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning.
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic.
I find inheriting without virtual functions quite useful. The main reason for the virtual destructor rule goes back to Scott Meyers Effective C++. He points out that if you have Base <-- Derived and you call delete Base* the destructor of Derived is not called. In general, it's hard to ensure your derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it.
is_polymorphic doesn't do anything so evil as to invoke UB, because the type created is only used to compute the compile-time value of the is_polymorphic predicate, and is never allocated on the heap or delete'd. Anyway, there are many other legitimate idioms that also don't unvoke UB, such as CRTP. Sometimes the existence of a base class is an implementation detail. The base class isn't documented as such, and there's nothing useful clients can do with it, so they never end up touching anything but the complete object. Inheritance is often a crucial tool in creating lightweight objects that shouldn't have any virtual functions.
As a practical example, my super string 'experiment' uses this technique for interface compatibility with std::string. In one incarnation super_string inherits from std::string, which doesn't have a virtual destructor. Since super_string only adds functions to the type, has no state, and a trivial destructor I don't care that it's not called. So, I have an extended type with functions useful to me, it's backward compatible with the existing type, and no virtual table/destructor is needed.
http://www.crystalclearsoftware.com/libraries/super_string/index.html
As long as you don't tell people a super_string IS-A std::string, you're arguably okay... though even then, they'll still probably end up invoking UB unintentionally. -- Dave Abrahams Boost Consulting www.boost-consulting.com Don't Miss BoostCon 2007! ==> http://www.boostcon.com
David Abrahams wrote:
on Sat May 05 2007, Jeff Garland <jeff-AT-crystalclearsoftware.com> wrote:
Jonathan Franklin wrote:
For example, GCC has a
warning about a derived class whose base doesn't have a virtual dtor. It's actually *impossible* (not just inefficient or convoluted) to implement is_polymorphic without generating that warning.
Interesting. I'm obviously flaunting my ignorance, but I didn't realize that inheriting from a class sans virtual dtor was ever a Good Thing. I'll have to read up on the issues WRT is_polymorphic.
I find inheriting without virtual functions quite useful. The main reason for the virtual destructor rule goes back to Scott Meyers Effective C++. He points out that if you have Base <-- Derived and you call delete Base* the destructor of Derived is not called. In general, it's hard to ensure your derived classes won't need the destructor called and hence the rule. However, if Derived happens to have a trivial/empty destructor then, really there's no reason to call it.
is_polymorphic doesn't do anything so evil as to invoke UB, because the type created is only used to compute the compile-time value of the is_polymorphic predicate, and is never allocated on the heap or delete'd.
Anyway, there are many other legitimate idioms that also don't unvoke UB, such as CRTP. Sometimes the existence of a base class is an implementation detail. The base class isn't documented as such, and there's nothing useful clients can do with it, so they never end up touching anything but the complete object. Inheritance is often a crucial tool in creating lightweight objects that shouldn't have any virtual functions.
And what wrong does gcc do with CRTP? As far I can tell, if you use -Wall, the warning is only emitted for class that has virtual functions and non-virtual destructor. For that case, adding virtual destructor is cheap, as there's already vtable, and seem to be good thing to do. I've tried this example with is_polymorphic: #include <boost/type_traits/is_polymorphic.hpp> struct C1 { int i; }; struct C2 { virtual void foo() {} }; int main() { return boost::is_polymorphic<C1>::value + boost::is_polymorphic<C2>::value; } And gcc warns. Desprite warning being overly wordly, it basically warns that C2 has virtual function but lacks virtual destructor. Adding virtual destructor to C2 makes the example compile without warning. Or maybe you have -Weffc++ option in mind? That, indeed, causes warning to be emitted for all base classes without virtual destructors. However, nobody uses that option, precisely because it gives pages and pages of warnings on legitimate code, including standard library. - Volodya
On 5/6/07, Vladimir Prus <ghost@cs.msu.su> wrote:
And gcc warns. Desprite warning being overly wordly, it basically warns that C2 has virtual function but lacks virtual destructor. Adding virtual destructor to C2 makes the example compile without warning.
- Volodya
The gcc warning bothers me at times. Particularly in cases like this: struct Callback { virtual int someCall(Foo foo) = 0; }; void someFunc(Callback * callback); As shown, the code above is pretty obvious (I hope) - implement a 'Callback' and someFunc will call it with a Foo (for whatever reason). If I now add a virtual destructor to Callback, am I implying that someFunc will also delete the callback?! ie adding the virtual destructor says 'you can delete this from the base' but I don't always want to say that. (And yes, in this case I could use a boost::function, or a Callback & instead of a pointer, etc, but it is just meant to be a simple example of similar cases that I've seen in actual code.) The solution we came up with is to include the virtual destructor, but make it private. And add a comment that the destructor is there to quiet gcc, but it is private to imply that no one is going to delete via the base. Tony
David Abrahams wrote:
on Sat May 05 2007, Jeff Garland <jeff-AT-crystalclearsoftware.com> wrote:
As a practical example, my super string 'experiment' uses this technique for interface compatibility with std::string. In one incarnation super_string inherits from std::string, which doesn't have a virtual destructor. Since super_string only adds functions to the type, has no state, and a trivial destructor I don't care that it's not called. So, I have an extended type with functions useful to me, it's backward compatible with the existing type, and no virtual table/destructor is needed.
http://www.crystalclearsoftware.com/libraries/super_string/index.html
As long as you don't tell people a super_string IS-A std::string, you're arguably okay... though even then, they'll still probably end up invoking UB unintentionally.
I actually don't think it's too important what I say. Most code won't invoke the UB, but even if the user strays into the UB it's harmless. Technically there is a risk that some future compiler might change the UB into bad behavior, but that risk seems small. All this is making me wonder if there isn't some reason that this has to be UB since all the compilers have the same solution... Jeff
Sohail Somani wrote: [...]
If you treat warnings as errors and if bjam has an option to "keep going" as much as possible, then you get builds as far as they can go (i.e. all the warnings) and builds fail if there are warnings. Then I think you don't need to depend on the regression reporting tools for the rest of time, atleast for this stuff.
I'm convinced that library authors already make an effort to clear warnings on the platforms they have available. The real problem is dealing with platforms not available to developers. For those you do need to rely on regression reports and feedback after a tentative patch can take as long as a whole week. Furthermore I'm sure that library authors already devote to Boost all the time they can afford.
Usually the warnings I've seen in Boost are pretty easy to fix but its understandable that that puts yet another load on the developers. The "problem" with patches is that they take forever to get accepted and/or commented on as they are lots of little loads and resources are limited. Warning-fixing patches are understandably lower priority than segfault-fixing ones. Maybe a catch-all-warning-fixes bug is in order if we can get some sort of resource with commit privilege to look over it? The prerequisite is that the patches should ONLY quiet warnings.
One improvement would be to move to a bug tracking tool more manageable than sourceforge's. As a new Trac is being setup, maybe the plan is to move to its own tool, but I don't know for sure and I'm not familiar with it.
If we do this, and then at some point accept this catch-all set of patches, then we can turn on warnings as errors once the patches have been merged. I've had to do this before, and this works as far as setting you up for the future.
For how many platforms at the same time? I don't see this working unless you're able to ensure nightly regression testing turnaround for all the supported platforms. Cheers, Nicola Musatti
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Nicola Musatti Sent: Wednesday, May 02, 2007 1:53 PM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
I'm convinced that library authors already make an effort to clear warnings on the platforms they have available. The real problem is dealing with platforms not available to developers. For those you do need to rely on regression reports and feedback after a tentative patch can take as long as a whole week. Furthermore I'm sure that library authors already devote to Boost all the time they can afford.
Exactly, which is why I suggested a fix-all-warnings bug. [snip]
If we do this, and then at some point accept this catch-all set of patches, then we can turn on warnings as errors once the patches have been merged. I've had to do this before, and this works as far as setting you up for the future.
For how many platforms at the same time? I don't see this working unless you're able to ensure nightly regression testing turnaround for all the supported platforms.
It sounds like gcc/msvc are the ones people would be most happy with. By turnaround, do you mean them actually running? I don't see why it wouldn't be possible for a subset of configurations. Sohail
Sohail Somani wrote: [...]
It sounds like gcc/msvc are the ones people would be most happy with. By turnaround, do you mean them actually running? I don't see why it wouldn't be possible for a subset of configurations.
Considering the 1.34 regression tests, which only cover officially supported platforms, between gcc and msvc there are 16 variants. You may decide to cover only a subset, but you must test all the others to ensure you didn't break anything. I wish I could afford to run so many regression tests each night. I still think that a revived bug traking tool, where people who care post warning clearing patches, is more viable. Cheers, Nicola Musatti
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Nicola Musatti Sent: Wednesday, May 02, 2007 2:41 PM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Sohail Somani wrote: [...]
It sounds like gcc/msvc are the ones people would be most happy with. By turnaround, do you mean them actually running? I don't see why it wouldn't be possible for a subset of configurations.
Considering the 1.34 regression tests, which only cover officially supported platforms, between gcc and msvc there are 16 variants. You may decide to cover only a subset, but you must test all the others to ensure you didn't break anything. I wish I could afford to run so many regression tests each night.
I still think that a revived bug traking tool, where people who care post warning clearing patches, is more viable.
Sure, it's the acceptance of those patches which is a trouble (though I may be wrong) Sohail
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Nicola Musatti Sent: Wednesday, May 02, 2007 2:41 PM To: boost@lists.boost.org Subject: Re: [boost] [1.34.0beta] many, many warnings... :(
Sohail Somani wrote: [...]
It sounds like gcc/msvc are the ones people would be most happy with. By turnaround, do you mean them actually running? I don't see why it wouldn't be possible for a subset of configurations.
Considering the 1.34 regression tests, which only cover officially supported platforms, between gcc and msvc there are 16 variants. You may decide to cover only a subset, but you must test all the others to ensure you didn't break anything. I wish I could afford to run so many regression tests each night.
I still think that a revived bug traking tool, where people who care post warning clearing patches, is more viable.
You know what, disregard my other post. I think I'm not being fair. I'm mixing up "build without warnings" with other stuff but its been that kind of day. Apologies. Sohail
Nicola Musatti said: (by the date of Wed, 02 May 2007 23:40:57 +0200)
Sohail Somani wrote: [...] Considering the 1.34 regression tests, which only cover officially supported platforms, between gcc and msvc there are 16 variants. You may decide to cover only a subset, but you must test all the others to ensure you didn't break anything. I wish I could afford to run so many regression tests each night.
I still think that a revived bug traking tool, where people who care post warning clearing patches, is more viable.
IMHO a following statement should be put into 1.34 release notes. And perhaps even on the boost front page in news regardin the release: "We are working to remove unnecessary warnings caused by boost headers during compilation. This task is not easy due to big number of supported compilers and removing a warning in one of them causes a warning in the other compiler. Moreover the regression tests are unable to catch all possible warnings - they mostly come up during regular usage. We kindly ask our users to submit "remove warning" patches to the bugtracker. We hope that by this action the next release will have a much smaller amount of warnings" -- Janek Kozicki |
participants (20)
-
"JOAQUIN LOPEZ MU?Z" -
Beman Dawes -
Bo Persson -
Boris Gubenko -
David Abrahams -
Domenico Andreoli -
Gottlob Frege -
Janek Kozicki -
Jeff Garland -
Jody Hagins -
Jonathan Franklin -
Marc Mutz -
Marc Mutz -
Nicola Musatti -
Olaf Krzikalla -
Paul Giaccone -
Sebastian Redl -
Sohail Somani -
Stefan Seefeld -
Vladimir Prus