Re: [PATCH] Use OPT_CALLBACK and OPT_CALLBACK_F

From: Jeff King <peff@peff.net>
Date: Apr 28, 2020, 08:45 UTC
Message-ID: <20200428084543.GC2381876@coredump.intra.peff.net>
In-Reply-To: <d6d35a21a5bb1077eea4dd49f5716bece9e015de.1588062647.git.liu.denton@gmail.com>

On Tue, Apr 28, 2020 at 04:36:28AM -0400, Denton Liu wrote:

Show 6 quoted lines

> In the codebase, there are many options which use OPTION_CALLBACK in a
> plain ol' struct definition. However, we have the OPT_CALLBACK and
> OPT_CALLBACK_F macros which are meant to abstract these plain struct
> definitions away. These macros are useful as they semantically signal to
> developers that these are just normal callback option with nothing fancy
> happening.

I think this is worth doing. It's a little easier to read, and sets a better example anyone copying the code.

> Replace plain struct definitions of OPTION_CALLBACK with OPT_CALLBACK or
> OPT_CALLBACK_F where applicable. The heavy lifting was done using the
> following (disgusting) shell script:
> [...]

I'll admit I gave only a quick read through the results. I think between your script, the manual look-over for style, and the fact that the compiler would catch any minor syntactic screwups, I'm not likely to see any new errors.

> I contemplated breaking this down into file-sized patches but I don't
> think it really makes sense in this case since it's the same change
> which is being made in each file and, imo, it wouldn't really ease
> reviewer burden since the same number of changes are being reviewed.

As a reviewer I much prefer one big patch like this, since these are all the same case: changing style X to style Y. I'd want patches broken out if there were other case (say, the parameters for some version needed fixed up as you were converting). But I don't see that here.

-Peff