Re: [boost] Official warnings policy?

Subject: Re: [boost] Official warnings policy?
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2009-11-06 08:52:08

Gottlob Frege wrote:
>
> 1. Being at 0 warnings is worth it.  Important to remember.
> 2. Once you get there, stay there.  Much easier than getting there.
> 3. Fixing warnings can fix bugs.
> 4. Fixing warnings can *cause* bugs.
>     For large 'sprints' of warning fixes, I'd say it causes more bugs
> than it finds.  Way more.  Once you get to 0, warnings will find more
> bugs than they cause, but not during the drive to 0.  The key here is
> to make sure the person fixing the warning knows the code well.  Just
> because you fixed the warning the same way the last 10 times, it
> doesn't mean the next occurrence will work the same way.  And again,
> once you get to 0, new warnings will be fixed by their instigator, and
> fixed when the code is fresh in their minds - much more likely to be
> correct.

This sort of information would be useful in the Wiki policy Paul started as would some of the following.

> Some specific warnings, and our conclusions:
>
> - missing virtual dtor
>    - didn't find any bugs in our code, but it is not a bad warning in
> general, when you think about who might be using/maintaining your code
> in the future.

That's an excellent rationale.  You may handle it well in the current code, but a user may be concerned, may use your code in an unexpected way, or a maintainer may violate your design idea.

>    - solution:  add virtual dtor, make it protected, leave a comment
> about the warning

That's a bit vague.  The policy should be public and virtual or protected and non-virtual.  If the latter leads to a warning from a compiler, we should document the manner by which to quiet that compiler.  We could even create a pair of macros to bracket the protected, non-virtual dtor or use a macro to generate the declaration with the warning suppression.

> - missing enum values in switch case
>    - didn't find any bugs
>    - seemed like a nice idea at first - lets you know if you missed a
> case.  We tried various ways to restructure our code to take advantage
> of this warning (ie write the code so that if a new enum value was
> added you would get a warning about the switch).  However, more often
> than not, the restructuring introduced bugs, or made the code more
> convoluted or ... etc (typically with how to deal with "default:"
> case).  Maybe we were being "too smart by half" trying to make use of
> the warning.
>    - I'd disable this warning if I could, otherwise I'd fix the code

I use this warning to advantage.  I add a case for each enumerator and use the default case to assert, log an error, throw an exception, etc.  It ensures that I consciously address each enumerator.  That *has* alerted me to the need to handle a new enumerator.  Even if it could have been handled by a different default case, having to add a new case for the new enumerator forces me to consciously choose how to handle that enumerator.  That's a good thing.

> - order of member initialization list does not match order
> actual initialization
>   (ie members are initialized in the order listed inside the class,
> not the order in the constructor mem-init list)
>    - didn't find any bugs
>    - you shouldn't rely on member init order, and if you ever need to,
> you should really comment it well
>    - reordering your mem-int lists is an annoying waste of time
>    - the warning basically says "hey, know your language".  Should we
> warn whenever you use the comma operator or ?: because they esoteric
> parts of the language? (I once worked somewhere where these were
> banned for that reason...)
>    - I'd disable this warning if I could, otherwise I'd fix the code

Keeping the initializer list in good order means you can't be confused about the order of initialization.  I *have* found bugs in others' code because of this.  Many developers are ignorant of the order of initialization and can be bitten by it.  If such developers are trained to honor warnings, then this warning will help them.  At the very least, they will ask me about it.

The fix for this warning is so trivial that there shouldn't be an issue with it.  Fix the initializer list order and be done with it.

I order my data members alphabetically within each access section to make initializer list ordering easier.  (When object size matters, I will reorder the data members, of course, and then adjust the initializer lists accordingly.)

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.