Re: [lmi] Fall through

[Vadim Zeitlin]

On Fri, 11 Dec 2015 15:23:49 +0000 Greg Chicares <address@hidden> wrote:

GC> >  I don't know of any tools flagging if/else-if chains without the final
GC> > else as an error
GC> 
GC> I'm really surprised.

 Notice that I don't claim that no such tools exist, but I definitely
didn't see this in any compiler nor static analysis tool I used so far
(although I typically don't use the latter with the strictest options as
they result in too much noise, so perhaps they can do it if configured).

GC> And what about the missing cases...exactly what falls through? For the
GC> inner if...else chain, we examine 'tn_range_types.hpp' and see
GC>   {int, double, calendar_date}
GC> so the inner chain is exhaustive, for now.

 Personally I would use boost::variant<int, double, calendar_date> and its
visitor function to ensure that it's exhaustive at compile-time.

GC> Therefore, we should replace
GC> the null else with at least a warning. As for the outer if...else chain,
GC> at the bottom of 'input.hpp' we see:
GC> 
GC>         z = exact_cast<ce_product_name         >(m); if(z) return z;
GC>         z = exact_cast<datum_string            >(m); if(z) return z;
GC>         z = reconstitutor<datum_sequence,Input>::reconstitute(m); if(z) 
return z;
GC>         z = reconstitutor<mc_enum_base  ,Input>::reconstitute(m); if(z) 
return z;
GC>         z = reconstitutor<tn_range_base ,Input>::reconstitute(m); if(z) 
return z;
GC> 
GC> and obviously 'ce_product_name' is missing. I believe we've mentioned
GC> that before on this mailing list, but we haven't fixed it, so we should
GC> at least mark it inline as a known defect.

 I'm not sure about this. The intention was clearly to use fallback
converter in this case, so there would only be a defect here if this
converter didn't behave correctly for ce_product_name.

GC> >  And while it's hard to do it in this particular case (it would be simpler
GC> > with C++11...), generally speaking I prefer using switch() to chains of
GC> > if() statements as you can rely on the compiler checking the completeness
GC> > for you as both clang and g++ will also warn you about the missing cases 
if
GC> > you switch over an enum. This still doesn't make C++ switch() as nice as
GC> > pattern matching in some other languages, but it gets a little bit closer
GC> > to it.
GC> 
GC> I'm not so fond of it because 'break' is normally wanted, but must be
GC> written explicitly.

 If the compiler warns you when it isn't, this is not a problem -- which is
why the clang warning with which I started this (sub)thread is so nice.

GC> I like the ternary operator best of all, in the limited cases where it's
GC> appropriate, e.g.:
GC> 
GC>     return
GC>           (PC_24 == pc) ? fe_fltprec
GC>         : (PC_53 == pc) ? fe_dblprec
GC>         : (PC_64 == pc) ? fe_ldblprec
GC>         : throw std::runtime_error("Failed to determine hardware 
precision.")
GC>         ;

 I would absolutely use a switch here with an enum containing PC_XX
constants to ensure that adding some new PC_128 would result in a
_compile_- rather than run-, time error. Of course, one doesn't prevent the
other and when using a compiler without support for -Wswitch-enum, such as
pre-historic versions of gcc, a run-time error would still be generated:

        e_ieee754_precision convert_precision(e_pc_nn pcnn)
        {
                switch (pcnn)
                        {
                        case e_PC_24: return fe_fltprec;
                        case e_PC_53: return fe_dblprec;
                        case e_PC_64: return fe_ldblprec;
                        }

                throw std::runtime_error("...");
        }

 In this particular case it's a bit stupid, of course, as you would need to
define another enum (e_pc_nn) first, so you don't really gain much. But if
an enum already exists, e.g. when doing something with e_ieee754_precision
values, writing a switch over it is definitely safer than using the ternary
operator.

 Regards,
VZ