Re: [lmi] [lmi-commits] [6610] Do not attempt to define an average for "additional" premium

[Vadim Zeitlin]

On Fri, 20 May 2016 16:40:21 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-20 15:44, Vadim Zeitlin wrote:
GC> > On Fri, 20 May 2016 12:47:43 +0000 (UTC) address@hidden wrote:
GC> >>          std::string average_text;
GC> >> -        switch(col)
GC> >> +
GC> >> +        // The cast is only used to ensure that if any new elements are 
added
GC> >> +        // to the enum, the compiler would warn about their values not 
being
GC> >> +        // present in this switch.
GC> >> +        switch(static_cast<enum_group_quote_columns>(col))
GC> > 
GC> >  This comment doesn't make sense in conjunction with the "default" label
GC> > below, the warning won't be given if there is a "default" label, so either
GC> > this comment (and the cast) should be removed or (probably preferably) the
GC> > "default" label replaced with all the cases for the columns not handled.
GC> 
GC> Isn't this the same logic as is used in the places I copied it from, i.e.,
GC> the 'switch' statements on lines 787 and 971?

 I've already deleted my original branch(es) with these changes, so I can't
check easily, but I'm not sure if it was me who added "default" to these
switches. Even if it was, it was still wrong, either the cast or the
"default" needs to go as the former is useless in presence of the latter.

GC> In the past, I've often written 'default:' in every switch, not to specify
GC> a default action, but to catch omitted cases: a runtime error that is shown
GC> only on actual violations is better than no diagnostic. I adopted this habit
GC> in ancient times when compilers didn't offer an unmentioned-case warning.

 Yes, so did I.

GC> A compiler warning is preferable because it is shown at compile time and
GC> presumably whenever a violation is possible, whether or not it occurs.

 Exactly, and lmi can rely on being compiled with a compiler which will
always give this warning, so runtime check is not necessary any more
(unlike in e.g. wx code which may still be compiled with compilers that
don't warn about this).

GC> Now, we have two available warnings, '-Wswitch' and '-Wswitch-enum':
GC>   
https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Warning-Options.html#Warning-Options
GC> We must already be using '-Wswitch' because '-Wall' turns it on. We aren't
GC> using '-Wswitch-enum' (it gives this warning even if a 'default:' case is
GC> specified).

 IME -Wswitch-enum is often annoying, its effective result is to force you
to stop using "default" completely as it very rarely makes sense to list
all the cases and still have the default clause. But I'll try building lmi
with it and will let you know about the results.

 Regards,
VZ