On Thu, Oct 10, 2019 at 11:44:39AM -0700, Jonathan Tan wrote:
> > 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.
>
> > - 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.
> > -#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.
> 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!
