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

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

quoted

quoted

-/*
- * 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