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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Oct 7, 2019, 17:29 UTC
Message-ID: <20191007172951.GC11529@szeder.dev>
In-Reply-To: <xmqqlfu9krzv.fsf@gitster-ct.c.googlers.com>

On Sat, Sep 28, 2019 at 12:40:52PM +0900, Junio C Hamano wrote:

Show 36 quoted lines

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Fri, Sep 20, 2019 at 09:53:48AM -0700, William Baker via GitGitGadget wrote:
> >> diff --git a/midx.h b/midx.h
> >> index f0ae656b5d..e6fa356b5c 100644
> >> --- a/midx.h
> >> +++ b/midx.h
> >> @@ -37,6 +37,8 @@ struct multi_pack_index {
> >>  	char object_dir[FLEX_ARRAY];
> >>  };
> >>  
> >> +#define MIDX_PROGRESS     (1 << 0)
> >
> > Please consider using an enum.
> 
> If they are used by assiging one of their values, definitely a good
> idea to use an enum.  Are debuggers clever enough that they can
> tell, when they see something like this:
> 
> 	enum gress {
> 		PROGRESS = 1,
> 		REGRESS = 2,
> 	};
> 
> 	void func(enum gress v);
> 
> 	...
> 
>         void caller(void)
> 	{
> 		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. 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 (e.g. after someone changes the value of REGRESS from 2 to 8, 'PROGRESS * 3' will mean something different).