[lmi] mcenum_emission: bit mask or not?

[Vadim Zeitlin]

 Hello,

 I need to refactor the existing [pre_]emit_ledger() functions in order to
allow preserving some state across the calls to them which is needed for
the premiums PDF generation. This is not difficult on its own and I've
already done it by replacing these functions with a ledger_emitter class
with add_ledger() method.

 However there is quite a bit of extra complexity in this class currently
due to the fact that I followed the comment for mcenum_emission which says

        /// Enumerators are binary powers so that more than one can be
        /// specified in a single scalar entity.

This means that a single instance of ledger_emitter, as currently written,
supports emitting a ledger to, say, printer (as PDF) and spreadsheet at
once.

 The problematic part, for me, is that this possibility is only used in a
single place in the entire code base, namely in the illustrator class when
creating an .inix output file. IMO the code there, which currently looks
like this:

        mcenum_emission x = emit_pdf_too ? mce_emit_pdf_file : mce_emit_nothing;
        emit_ledger(..., static_cast<mcenum_emission>(x | emission_));

would be better rewritten in the following more natural way:

        emit_ledger(..., emission_);
        if(emit_pdf_too)
                {
                emit_ledger(..., mce_emit_pdf_file);
                }

which avoids an extra variable and an ugly static_cast<>. And if this were
done, mcenum_emission could become a normal, safer to use (because there
would be no more need for static_casts anywhere) enum.

 Of course, this still leaves mce_emit_composite_only, mce_emit_quietly and
mce_emit_timings which are clearly meant to be combined with other enum
elements. But mce_emit_timings is not used at all anywhere and so should be
just removed. As for the other two, they are used only in main_cgi.cpp and
it would be trivial for me to update the code there to use separate methods
of the new ledger_emitter class to allow suppressing output and/or
non-composite ledgers emission instead of these flags.


 To summarize, I propose to replace the current mcenum_emission bit mask
enum with a normal enum and remove the only 3 elements in it for which it
makes sense to use them as bit masks. The advantages are:

1. Type safety (at least as much as C++98 enums provide, but it's still
   better than nothing and if this change is done, it would be simple[r]
   to switch to C++11 class enums later, which are really safe).

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

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

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

 I don't see any disadvantages to doing what I propose but, obviously, this
is another change that you would need to integrate, so if you think the
benefits don't justify spending your time on it, this would be
disadvantageous enough to not do it. However the changes would be
relatively small and the sum of these changes and the patch adding
ledger_emitter (which I really need for the premiums PDF generation, there
is no way around it) handling just a single emission would be simpler than
just the ledger_emitter patch treating the emissions as bit masks.

 Please let me know what do you think,
VZ