Re: [PATCH 6/8] grep API: call grep_config() after grep_init()

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 10, 2021, 00:18 UTC
Message-ID: <YYsPySOHR8h+CYbl@nand.local>
In-Reply-To: <211109.86a6iedqpf.gmgdl@evledraar.gmail.com>

On Tue, Nov 09, 2021 at 03:06:22AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 31 quoted lines

> >> -/*
> >> - * Initialize one instance of grep_opt and copy the
> >> - * default values from the template we read the configuration
> >> - * information in an earlier call to git_config(grep_config).
> >> - */
> >>  void grep_init(struct grep_opt *opt, struct repository *repo)
> >>  {
> >> -	*opt = grep_defaults;
> >> +	struct grep_opt blank = GREP_OPT_INIT;
> >> +	memcpy(opt, &blank, sizeof(*opt));
> >
> > I'm nit-picking, but creating a throwaway struct for the convenience of
> > using designated initialization (at the cost of having to memcpy an
> > entire struct around) seems like overkill.
> >
> > Especially since we're just going to write into the other fields of the
> > the target struct anyway, I'd probably rather have seen everything
> > written out explicitly without the throwaway or memcpy.
>
> It's a widely used pattern in the codebase at this point, see
> 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT
> macro, 2021-07-01) (mine, but I stole it from Jeff King).
>
> As his linked-to compiler test shows the memcpy() is optimized away, so
> modern compilers will treat these idioms the same way.
>
> There was a suggestions somewhere that we should prorably move to that
> "*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the
> board, it would be less verbose. But I haven't tested if it's as widely
> supported, so I've just been sticking with that blank/memcpy() pattern
> for "do init in terms of macro".

I do at least prefer memcpy() over *<x> = <y> when x and y are structures. But I wasn't aware that this was common in our codebase. Anyway, my suggestion was only along the lines of "you're already writing individual fields below, so why not just do that throughout instead of memcpy()-ing some of them via a macro which expands to a designated initializer?"

But this is a cosmetic point, so whatever you feel fits in most with the surrounding style (so long as the pattern we're propagating isn't terrible, which is the case here) then I'm OK with it.

Thanks, Taylor