Re: [lmi] Fall through

[Greg Chicares]

On 2015-12-11 13:43, Vadim Zeitlin wrote:
> [Let me address just this part of your message separately:]
> 
> On Fri, 11 Dec 2015 13:34:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> As for this part:
> GC>      if(...
> GC>      ...
> GC>      else if(...
> GC>      ...
> GC>          {
> GC> -        // else: fall through
> GC> +        }
> GC> +    else
> GC> +        {
> GC> +        ; // Fall through.
> GC>          }
> GC> I figure that it's preferable to express a control structure in code
> GC> rather than comments. Perhaps someday we'll use an automated tool
> GC> that looks for code smells like 'if...else if...else if' with no
> GC> catch-all 'else' at the end.
> 
>  I don't know of any tools flagging if/else-if chains without the final
> else as an error

I'm really surprised. I thought that was a universally acknowledged best
practice. It's in Kernighan and Plauger, _The Elements of Programming Style_:
  Avoid THEN-IF and null ELSE.

But look again at the change I committed--I misunderstood the structure.

http://svn.savannah.nongnu.org/viewvc/lmi/trunk/census_view.cpp?root=lmi&r1=6434&r2=6433&pathrev=6434&diff_format=u

+    else if(is_reconstitutable_as<tn_range_base >(value))
         {
         tn_range_base const* as_range = member_cast<tn_range_base>(value);
         if(typeid(int) == as_range->value_type())
@@ -687,7 +685,10 @@
             return get_impl<renderer_double_range_convertor>();
         else if(typeid(calendar_date) == as_range->value_type())
             return get_impl<renderer_date_convertor>();
-        // else: fall through
+        }
+    else
+        {
+        ; // Fall through.
         }

To which if...else chain does the original "// else: fall through" apply?

What would my hypothetical "missing-else-at-the-end" tool say?

And what about the missing cases...exactly what falls through? For the
inner if...else chain, we examine 'tn_range_types.hpp' and see
  {int, double, calendar_date}
so the inner chain is exhaustive, for now. Therefore, we should replace
the null else with at least a warning. As for the outer if...else chain,
at the bottom of 'input.hpp' we see:

        z = exact_cast<ce_product_name         >(m); if(z) return z;
        z = exact_cast<datum_string            >(m); if(z) return z;
        z = reconstitutor<datum_sequence,Input>::reconstitute(m); if(z) return 
z;
        z = reconstitutor<mc_enum_base  ,Input>::reconstitute(m); if(z) return 
z;
        z = reconstitutor<tn_range_base ,Input>::reconstitute(m); if(z) return 
z;

and obviously 'ce_product_name' is missing. I believe we've mentioned
that before on this mailing list, but we haven't fixed it, so we should
at least mark it inline as a known defect.

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

I'm not so fond of it because 'break' is normally wanted, but must be
written explicitly.

I like the ternary operator best of all, in the limited cases where it's
appropriate, e.g.:

    return
          (PC_24 == pc) ? fe_fltprec
        : (PC_53 == pc) ? fe_dblprec
        : (PC_64 == pc) ? fe_ldblprec
        : throw std::runtime_error("Failed to determine hardware precision.")
        ;