Re: [PATCH v2 1/4] i18n: factorize more 'incompatible options' messages

From: Jean-Noël AVILA <jn.avila@free.fr>
Date: Jan 29, 2022, 10:41 UTC
Message-ID: <8789935.eFtBeim4Pn@cayenne>
In-Reply-To: <f7468ea3-d9f6-96be-168d-de610d535e72@kdbg.org>

Le samedi 29 janvier 2022, 09:08:34 CET Johannes Sixt a écrit :

Show 18 quoted lines

> Am 29.01.22 um 00:58 schrieb Junio C Hamano:
> > Johannes Sixt <j6t@kdbg.org> writes:
> >> A bit of code duplication could be avoided if die_if_incompatible_opt3()
> >> forwarded with an additional pair 0, "" to die_if_incompatible_opt4().
> > 
> > I wondered if a single varargs function
> > 
> >     void die_if_incompatible_optN(const char *name1, int opt1, ...);
> > 
> > that takes a name=NULL terminated sequence of <name, opt> would
> > work, but
> > 
> >  (1) it would require the caller to always spell out the terminating
> >      NULL, which may be ugly.
> > 
> >  (2) it would tempt people to programatically generate message for N
> >      options, which leads to sentence lego, which is the exact
> >      opposite of what this series wants to achieve.

This is the second part of the functions, and they need a finite number of args to check in order to use the correct i18n string. This function cannot be made generic for any number of options to check.

Show 12 quoted lines

> 
> The reason I did not suggest a varargs version is because
> 
>   (3) it is not typesafe.
> 
> A varargs implementation could be used as an implementation helper, but
> should not be a public interface.
> 
> > In any case, I do agree with you that the callers of _opt3()
> > variants can just pass (0, "unused") in the 4-th slot.  If this were
> > made varargs, then it only needs to pass NULL at the end, so that
> > might be an improvement, though.

I still prefer to keep a public function signature which is explicit. Using varargs for alternating types is really not type safe.

In such case, declaring the functions with less options as inline calls to the one with the greatest number is a good balance.

Show 10 quoted lines

> > 
> > Also, isn't "if" in the name of the function misleading?  as far as
> > I can tell, this function is not "check if these options are
> > compatible and die if some are incompatible with each other", but
> > the caller is convinced that these options are incompatible before
> > it decides to call this function and there is no "if" in what this
> > function can do.
> 
> Good point.
>

I don't get the point: the function checks if incompatible options are present on the command line and dies in such case. The caller does not know whether the function will make the program die, it really depends on the result of the check performed in the function. In case there's no or only one of the options, the function does nothing.

JN