Re: [Outreachy] [PATCH] blame: Convert pickaxe_blame defined constants to enums

From: Wambui Karuga <wambui.karugax@gmail.com>
Date: Oct 11, 2019, 08:52 UTC
Message-ID: <20191011085217.GA8481@wambui>
In-Reply-To: <20191010184439.94173-1-jonathantanmy@google.com>

On Thu, Oct 10, 2019 at 11:44:39AM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> > Convert pickaxe_blame preprocessor constants in blame.h to an enum.
> > Also replace previous instances of the constants with the new enum values.
> 
> First of all, thanks for your initiative in finding a microproject and
> making a patch for it!
> 
> About your commit message title, I know that 50 characters is a soft
> limit, but we should adhere to it if possible. Also, in Git, the letter
> following the colon is usually in lowercase. So I would write it like:
> 
>   blame: make PICKAXE_BLAME_* an enum
> 
> (Feel free to use that or think of a different one.)

Okay, thanks for the suggestion. I'll use it.

Show 15 quoted lines

> 
> > -	if ((opt & PICKAXE_BLAME_COPY_HARDEST)
> > -	    || ((opt & PICKAXE_BLAME_COPY_HARDER)
> > +	if ((opt & BLAME_COPY_HARDEST)
> > +	    || ((opt & BLAME_COPY_HARDER)
> 
> Any reason why the names are renamed to omit "PICKAXE_"? In particular,
> these names are still global, so it is good to retain the extra context.
> 
> (This doesn't mean that you are wrong to remove them - I just gave my
> opinion, and a reason for my opinion. If you had a reason to remove
> them, you can mention that, and we can discuss this together. Or, if you
> read my reason and agree with it, you can say that and put the
> "PICKAXE_" back.)
>

I wasn't really sure about omitting the "PICKAXE_" prefix, but I looked
at some of the other defined enums and it seemed like what would act as
the prefix in #defines was only used in the enum declaration. For
example I looked at:
	enum apply_ws_error_action {
		nowarn_ws_error,
		warn_on_ws_error,
		die_on_ws_error,
		correct_ws_error
	};

For comparison, I took "apply_" as the prefix that would translate to "#define APPLY_" which isn't included in the member variables. I do agree about retaining the extra context though, so I can definitely put the "PICKAXE_" back.

Show 22 quoted lines

> > -#define PICKAXE_BLAME_MOVE		01
> > -#define PICKAXE_BLAME_COPY		02
> > -#define PICKAXE_BLAME_COPY_HARDER	04
> > -#define PICKAXE_BLAME_COPY_HARDEST	010
> > -
> >  #define BLAME_DEFAULT_MOVE_SCORE	20
> >  #define BLAME_DEFAULT_COPY_SCORE	40
> >  
> > +enum pickaxe_blame_action {
> > +	BLAME_MOVE = 01,
> > +	BLAME_COPY,
> > +	BLAME_COPY_HARDER = 04,
> > +	BLAME_COPY_HARDEST = 010,
> > +};
> 
> In Git, we often look at historical commits, so it is good to keep
> history as clean as possible. In particular, we shouldn't move things
> around unless we have another reason to. Here, for example, you are
> moving the constants from above BLAME_DEFAULT_* to below. You should
> move them back. (Or if you have a reason for moving, mention that and we
> can discuss it.)
>

I'll move them back. I have experience with all the "#define" constants being immediately after the "#includes" which is why I moved them, but I'll try to stick to the convention from now on.

> Also, I have a slight preference for putting "= 02" on the BLAME_COPY
> line but that is not necessary.
>

Noted.

Show 7 quoted lines

> Apart from all that, one thing that I expected in this patch is the
> changing of the type of local variables and parameters. For example, in
> blame.c, I would have expected find_copy_in_parent() (for example) to
> have its "opt" parameter changed from "int" to "enum
> pickaxe_blame_option". One of the reasons why we want to use enums is
> for type safety, and that can only be done if we use the enum type when
> possible.

I overlooked this, sorry for that. I'll send an updated patch with these corrections.

Thanks!