Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Oct 9, 2019, 01:32 UTC
Message-ID: <20191009013231.GF29845@szeder.dev>
In-Reply-To: <xmqqk19fn9jp.fsf@gitster-ct.c.googlers.com>

On Tue, Oct 08, 2019 at 01:30:34PM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> >> 		func(PROGRESS | REGRESS);
> >> 		func(PROGRESS + REGRESS);
> >> 		func(PROGRESS * 3);
> >> 	}
> >> 
> >> how caller came about to give 3?
> >
> > No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb
> > do.

I was wrong here, gdb does this, but lldb, unfortunately, doesn't; see my other reply in this thread.

Show 12 quoted lines

> OK.
> 
> >  If the enum has only constants with power-of-two values, then that
> > is the right way to write it, and the other two are asking for trouble
> 
> If the programmer and the debugger knows the constants are used to
> represent bits that can be or'ed together, what you say is correct,
> but that is entirely irrelevant.
> 
> What I was worried about is that the constants that are used to
> represent something that are *NOT* set of bits (hence "PROGRESS * 3"
> may be perfectly a reasonable thing for such an application)

I don't really see how that could be reasonable, it's prone to break when changing the values of the enum constants.

Show 9 quoted lines

> may be
> mistaken by an overly clever debugger and "3" may end up getting
> shown as "PROGRESS | REGRESS".  When there are only two constants
> (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that
> is to represent two bits that can be or'ed together, or it is an
> enumeration.
> 
> Until we gain the third constant, that is, at which time the third
> one may likely be 3 (if enumeration) or 4 (if bits).

Humans benefit from context: they understand the name of the enum type (e.g. does it end with "_flags"?), the name of the enum constants, and the comment above the enum's definition (if any), and can then infer whether those constants represent OR-able bits or not. If they can't find this out, then that enum is poorly named and/or documented, which should be fixed. As for the patch that I originally commented on, I would expect the enum to be called e.g. 'midx_flags', and thus already with that single constant in there it'll be clear that it is intended as a collection of related OR-able bits.

As for the debugger, if it sees a variable of an enum type whose value doesn't match any of the enum constants, then there are basically three possibilities:

  - All constants in that enum have power-of-two values.  In this case
    it's reasonable from the debugger to assume that those constants
    are OR'ed together, and is extremely helpful to display the value
    that way.

  - The constants are just a set of values (1, 2, 3, 42, etc).  In
    this case the variable shouldn't have a value that doesn't match
    one of the constants in the first place, and I would first suspect
    that the program might be buggy.

  - A "mostly" power-of-two enum might contain shorthand constants for
    combinations of a set of other constants, e.g.:

      enum flags {
              BIT0 = (1 << 0),
              BIT1 = (1 << 1),
              BIT2 = (1 << 2),

              FIRST_TWO = (BIT0 | BIT1),
      };
      enum flags f0 = BIT0;
      enum flags f1 = BIT0 | BIT1;
      enum flags f2 = BIT0 | BIT2;
      enum flags f3 = BIT0 | BIT1 | BIT2;

    In this case, sadly, gdb shows only matching constants:

      (gdb) p f0
      $1 = BIT0
      (gdb) p f1
      $2 = FIRST_TWO
      (gdb) p f2
      $3 = 5
      (gdb) p f3
      $4 = 7