Re: [PATCH v4 24/27] builtin/rebase: do not assign default backend to non-constant field

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2024, 16:11 UTC
Message-ID: <xmqq8qzjwerb.fsf@gitster.g>
In-Reply-To: <Zl_6anUl_C3W2j2Z@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 16 quoted lines

>> Personally I'd be happy with
>> 
>> -		.default_backend = "merge",		\
>> +		.default_backend = xstrdup("merge"),	\
>> 
>> rather than adding an init function. I do agree that adding
>> rebase_options_release() is a good idea and the rest of the changes look
>> good to me
>
> Do we have any other cases where we allocate inside of a `_INIT` style
> macro? If so, I'd go with that precedence and just allocate inside of
> the macro. But if we don't, then I think I'm leaning more towards the
> way I did it in this patch.
>
> Happy to be convinced otherwise, I don't really feel all that strongly
> about this. I'm merely aiming for the interface wth least surprises.

FWIW, I am OK either way.

Having _init() and _release() may look symmetrical and there _might_ be cases where an initialization with a simple _INIT macro may not suffice and we must have _init() function.

But otherwise, especially with .designated_initializers, the _INIT macro makes it slightly easier to read (but not all much for this particular case, whose initial values have too many non-zero values and pretty much everything must be spelled out).

Thanks.