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

[Vadim Zeitlin]

On Thu, 30 Jul 2015 01:09:25 +0000 Greg Chicares <address@hidden> wrote:

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

 Yes, sorry for not noticing this sooner, this was unfortunately impossible
to find with grep. I don't dispute at all that it is useful there but the
code there could just call emit_ledger() several times in a loop instead of
calling it once with a bitmask just as easily.

GC> >  Maybe I should amend my proposal to split mcenum_emission in two: one
GC> > containing most of its elements in a normal enum and another bit mask enum
GC> > with just mce_emit_{composite_only,quietly,timings}
GC> 
GC> I suspect mce_emit_test_data would also need to be included, which
GC> changes the semantics so that this bit mask enum would no longer contain
GC> only modifiers: mce_emit_test_data is a primary output for '.ill', but
GC> for '.mec' it's a modifier that triggers an extra, secondary output.
GC> 
GC> Perhaps the use of mce_emit_test_data for '.mec' (and '.gpt') constitutes
GC> bastardization and should therefore be reworked to use a new enumerator
GC> like mce_emit_extra_data, so that the proposed bit mask enum would make
GC> sense. But no one has complained about it or noticed the defect in its
GC> use that's mentioned later in this email, so it doesn't need to be fixed
GC> now; I just note it as a subtlety that shows that this is not necessarily
GC> a trivial refactoring exercise.

 Yes, it was too subtle for me to notice...

GC> I don't see a strong case for changing anything.

 No, the case is not strong and was half-built on the erroneous idea that
the different bits were not combined anywhere anyhow, so let me retire my
suggestion, the benefits (some extra type safety and simpler code in
emit_ledger.cpp) are not worth the danger of changing this non-trivial
code.

 FWIW I still believe that the use of bit masks here is not obvious at all
so I fully support

GC> or, probably much better, to change the name--I don't want to make it too
GC> much longer, like
GC>   s/mcenum_emission/mcenum_emission_bitfield/g
GC> but would
GC>   s/mcenum_emission/mcenum_emit_bits/g
GC> be enough to prevent such misunderstandings?

this idea.

GC> 5. Here's a bitfield hole I fell into:
GC>   /lmi/src/lmi[0]$grep '&& *emission' *.?pp
GC>   gpt_server.cpp:    if(mce_emit_test_data && emission_)
GC>   mec_server.cpp:    if(mce_emit_test_data && emission_)

 This just confirms that bit masks are inherently error-prone. I'd still
argue for not using them if we were discussing writing new code but, again,
this code already exists and it's probably better to leave it alone.

 Sorry for raising this subject unnecessarily,
VZ