Re: [lmi] Pagination anomaly

[Greg Chicares]

On 2018-02-06 14:21, Vadim Zeitlin wrote:
> On Tue, 6 Feb 2018 12:52:39 +0000 Greg Chicares <address@hidden> wrote:
> GC> [...] have we
> GC> agreed that it's generally preferable for enumerative 'switch'
> GC> statements to write a 'case' for every enumerator, and rely on the
> GC> compiler to identify any missing case?
> 
>  There are 2 possibilities: the first one is to do this, relying on the
> "default" (because included in -Wall) -Wswitch to detect the missing cases.
> The second one is to explicitly use -Wswitch-enum (which is not part of
> -Wall) to give this warning even if there is a "default" label. The only
> advantage of the latter is that it allows to detect the problem during
> run-time if it somehow slips through during compilation, but this shouldn't
> be possible in lmi, which always uses gcc with -Wall and -Werror. And the
> -Wswitch-enum option has a disadvantage of forcing to explicitly list all
> the enum elements even if all or many of them are handled in the same way
> (trying to build lmi with this option right now results in quite a few
> errors). So, for lmi, your proposal indeed seems preferable.

I hadn't thought of trying '-Wswitch-enum'. Of course we don't want to
use it regularly, but running it once, just to see what happens, was
interesting. For example, in 'ihs_avsolve.cpp', there's a
  switch(a_SolveType)
with a case for every enumerator except the one indicating that no solve
is desired. If no solve is desired, then this switch shouldn't be reached.
Adding that "impossible" case and treating it as a logic error enables
our customary '-Wswitch' to detect any other missing enumerator at run
time, which is an improvement.

> GC> The
> GC>     switch(output_mode)
> GC> statements in 'pdf_writer_wx.cpp' list every possible 'case' and
> GC> also 'default:'.
> 
>  I believe that I did it like this for consistency with the other switch
> statements in lmi

Of course--otherwise you would not have wished to invent the idiom;
and that's an argument in favor of uninventing it.

> GC> Maybe I should remove 'default:' wherever possible throughout
> GC> lmi, once and for all.
> 
>  Yes, I think it would be a good (even if not particularly life-changing,
> so probably not very urgent) thing to do.

Agreed, but now that I've sunk my teeth into it...sometimes all that
titanium just has a mind of its own.

> It also won't be completely
> trivial, some default labels can't be removed easily even if they look
> superficially similar to the ones that can. E.g. the switch in
> Ledger::SetRunBases() contains one which looks very similar to the one in
> pdf_writer_wx.cpp, but the switch there doesn't have cases for several enum
> elements (mce_variable_annuity and 3 more mce_xxx_obsolete values).

Rather than discuss this one theoretically, I'll just commit my idea
for making these two files similar and see what you think of it.

BTW...I know I'm supposed to have moved on to reviewing something
else now, but in 'group_quote_pdf_gen_wx.cpp':

    for(int col = 0; col < e_col_max; ++col)
        {
        column_definition const& cd = column_definitions[col];
        std::string header;

        // The cast is only used to ensure that if any new elements are added
        // to the enum, the compiler would warn about their values not being
        // present in this switch.
        switch(static_cast<enum_group_quote_columns>(col))

wouldn't it be better if 'col' were of type 'enum_group_quote_columns'?
I'm guessing you would have made it such, if only it were easy to iterate
across all enumerators. I'd like to attempt such a change, at least to
see if it looks any better; is defining an array that includes each
enumerative value, and using ranged for, still the least awful way of
doing this?