Re: [lmi] mcenum_emission: bit mask or not?

[Greg Chicares]

On 2015-07-05 18:03, Vadim Zeitlin wrote:
> On Sun, 5 Jul 2015 19:18:01 +0200 I wrote:
> 
> Me>  Of course, this still leaves mce_emit_composite_only, mce_emit_quietly 
> and
> Me> mce_emit_timings which are clearly meant to be combined with other enum
> Me> elements. But mce_emit_timings is not used at all anywhere and so should 
> be
> Me> just removed.
> 
>  Oops, sorry, actually trying removing it and running the tests showed the
> limits of my static code analysis[*] approach: it turns out that it is used
> via --emit=emit_timings option of main_cli.cpp, so it probably can't be
> removed finally.

Yes, the bitmask is quite useful for tests in makefiles:

/lmi/src/lmi[0]$grep emit workhorse.make
cli_test-sample.cns: special_emission := emit_composite_only
          --emit=$(special_emission),emit_text_stream,emit_quietly,emit_timings 
\
          --emit=$(special_emission),emit_text_stream,emit_quietly \
%.cns:  test_emission := emit_quietly,emit_test_data
%.ill:  test_emission := emit_quietly,emit_test_data
%.ini:  test_emission := emit_quietly,emit_custom_0
%.inix: test_emission := emit_quietly,emit_custom_1
%.mec:  test_emission := emit_quietly,emit_test_data
%.gpt:  test_emission := emit_quietly,emit_test_data
          --emit=$(test_emission) \

and I sometimes specify other combinations manually on the command line.

In 'main_cli.cpp', each '--emit' suboption is checked, so there's no
type-safety hole there:

        try
            {
            emission = mcenum_emission
                ( emission
                | mc_emission_from_string(token)
                );
            }
        catch(std::runtime_error const&)
            {
            std::cerr
                << argv[0]
                << ": unrecognized '--emit' suboption "
                << "'" << token << "'"
                << std::endl
                ;
            }

>  Maybe I should amend my proposal to split mcenum_emission in two: one
> containing most of its elements in a normal enum and another bit mask enum
> with just mce_emit_{composite_only,quietly,timings}

I suspect mce_emit_test_data would also need to be included, which
changes the semantics so that this bit mask enum would no longer contain
only modifiers: mce_emit_test_data is a primary output for '.ill', but
for '.mec' it's a modifier that triggers an extra, secondary output.

Perhaps the use of mce_emit_test_data for '.mec' (and '.gpt') constitutes
bastardization and should therefore be reworked to use a new enumerator
like mce_emit_extra_data, so that the proposed bit mask enum would make
sense. But no one has complained about it or noticed the defect in its
use that's mentioned later in this email, so it doesn't need to be fixed
now; I just note it as a subtlety that shows that this is not necessarily
a trivial refactoring exercise.

> that would be passed
> using an independent "int flags = 0" argument to the illustrator class
> ctor. This would minimize the amount of changes even further yet still have
> all the benefits mentioned in my original email.

I don't see a strong case for changing anything. Your original email said:

  http://lists.nongnu.org/archive/html/lmi/2015-07/msg00000.html
VZ | 1. Type safety (at least as much as C++98 enums provide, but it's still
VZ |    better than nothing and if this change is done, it would be simple[r]
VZ |    to switch to C++11 class enums later, which are really safe).

Of course that is an advantage in theory, but I don't really see any in
practice, given the validation mentioned above for command-line arguments.
Suppose you want to clone this function:

    void CensusView::UponRunCaseToGroupRoster(wxCommandEvent&)
    {
        DoAllCells(mce_emit_group_roster);
    }

for a new purpose, with a new "emission" argument. If you specify that new
argument as "mce_quarterly", then gcc complains:
  error: no matching function for call to `Cens usView::DoAllCells(mcenum_mode)'
Or if you use "131" as the argument, then gcc complains:
  error: no matching function for call to `CensusView::DoAllCells(int)'
What conceivable error could we want the compiler to detect that it does not
already detect?

VZ | 2. Code simplification: it's really easy to forget to account for the fact
VZ |    that mcenum_emission value can contain a combination of enum elements
VZ |    and not just be equal to one of them.

AFAICS this matters only in 'emit_ledger.cpp', and perhaps 'illustrator.cpp'
and 'main_c[gl]i.cpp'. In the few files where the bitfield-nature matters, the
use of '&' and '|' operators gives a hint that the value is a bitfield, so
I had thought that nothing more was needed. But if you can forget this, then
others can too, so I'd be willing to add a comment in, say, 'emit_ledger.hpp',
or, probably much better, to change the name--I don't want to make it too
much longer, like
  s/mcenum_emission/mcenum_emission_bitfield/g
but would
  s/mcenum_emission/mcenum_emit_bits/g
be enough to prevent such misunderstandings?

VZ | 3. Safer maintenance in the future: if my proposal is implemented, we would
VZ |    have a "switch" over mcenum_emission values and non-ancient versions of
VZ |    g++ (or clang and probably others too) would warn if a new enum element
VZ |    were added but the corresponding "case" not added, whereas now the
VZ |    compiler wouldn't be able to help me if I forget to add the code to
VZ |    handle the new mce_emit_pdf_premium element.

You'd add that code only in 'emit_ledger.cpp', right? And if you forgot to
add it, wouldn't you notice that right away in testing? If we had many such
switch statements in numerous source files, then I can see how a compiler
warning might be helpful; but in this case it seems like overkill to me.

VZ | 4. Last and really least: instead of being limited to log2(1.0+ULONG_MAX)
VZ |    emission types, you could have up to ULONG_MAX of them.

We haven't run out of enumerators yet, so we don't need to change anything
today for this reason.

One additional point:

5. Here's a bitfield hole I fell into:
  /lmi/src/lmi[0]$grep '&& *emission' *.?pp
  gpt_server.cpp:    if(mce_emit_test_data && emission_)
  mec_server.cpp:    if(mce_emit_test_data && emission_)
I'll have to fix that. I don't see this mistake as adequate reason to avoid
bitfields, though. If I took mistakes that way in general, there's hardly a
feature of any language that I'd permit myself to use.

VZ |  I don't see any disadvantages to doing what I propose but, obviously, this
VZ | is another change that you would need to integrate, so if you think the
VZ | benefits don't justify spending your time on it, this would be
VZ | disadvantageous enough to not do it.

The motivation here was that the bitfield at first seemed to be unnecessary,
but it turns out to be important...so I do think it's time to abandon this
idea so we can spend our time elsewhere.