Christian Couder <christian.couder@xxxxxxxxx> writes:
> From: Christian Couder <christian.couder@xxxxxxxxx>
>
> We often accept both a "--key" option and a "--key=<val>" option.
>
> These options currently are parsed using something like:
>
> if (!strcmp(arg, "--key")) {
> /* do something */
> } else if (skip_prefix(arg, "--key=", &arg)) {
> /* do something with arg */
> }
>
> which is a bit cumbersome compared to just:
>
> if (skip_to_opt_val(arg, "--key", &arg)) {
> /* do something with arg */
> }
Sounds sensible; especially if there are many existing code that can
be shortened by using this helper, that is.
> Note that, when using skip_to_opt_val(), it is not possible any more
> to do something different when the first argument is exactly "--key"
> than when it is exactly "--key=", but in most cases we already don't
> make any difference, which is a good thing.
Note that "it is not possible" is misleading. skip_to_opt_val()
could be written to allow the caller to tell the difference if you
chose to, but *you* made it impossible by assigning "" to arg upon
seeing "--key". You could assign NULL to arg in that case, and
"--key" and "--key=" can be differenciated by checking arg; the
former will give you NULL and the latter "".
Not that I am convinced that it is a bad idea to deliberately lose
information like this implementation does. At least not yet.
If there will be no conceivable caller that wants to differenticate
between the two, the implementation that "loses information" can
claim to be easier to use, as the callers do not have to be forced
to write something like:
if (skip_to_optional_val(arg, "--key", &arg)
do_something(arg ? arg : "");
to treat them the same.
Having said all that, I would expect skip_to_optional_val() (as a
name of the helper I suspect skip_to_optional_arg() is more
appropriate, though) to store NULL in the arg thing if there is no
optional argument given. Also, the caller does not have to even do
the 'arg ? arg : ""' dance if it is so common and natural for the
application to treat "--key" and "--key=" equivalently, as it is
expected that the actual code to work on the arg, i.e. do_something()
in the above example, _should_ be prepared to take NULL and "" and
treat them the same way (that is the definition of "it is so common
and natural for the application to treat them the same).
So, I think you identified interesting pattern that can be cleaned
up by introducing a helper, but I am not sure if I agree with the
exact design of the helper.
> Note that "opt" in the function name actually means "optional" as
> the function can be used to parse any "key=value" string where "key"
> is also considered as valid, not just command line options.
Yup. This paragraph is a good thing to have in the proposed log
message, to explain the reason why you force callers of this helper
to spell out the leading dashes like "--key" (not "key"). I think
that it is a sane design of the input side of this function---it
does not limit it to an overly narrow case of command line option
processing. For the same reason, I think it is saner design of the
output side to allow callers to tell between "key=" and "key".
While staring at this helper and writing "does not limit to command
line option processing", it occurs to me that a helper that allows
you to look for an optional ':' (instead of '=') may also be useful,
so the final version might become a pair of functions, perhaps like
so:
int skip_to_optional_delim(const char *s,
const char *prefix, char delim,
const char **rest)
{
const char *p;
if (!skip_prefix(str, prefix, &p))
return 0;
if (!*p)
*rest = NULL;
else if (*p != delim)
return 0;
else
*rest = p + 1;
return 1;
}
int skip_to_optional_arg(const char *s,
const char *prefix,
const char **arg)
{
return skip_to_optional_delim(s, prefix, '=', arg);
}
As the first one would not (by definition) have any callers
initially after your series, it can be static to a file that
implements both of them and it is OK to expose only the latter to
the public.
I do think it is a premature optimization to inline this function,
whose initial callers will be (from the look of the remainder of
your patches) command line parsing that sits farthest from any
performance critical code.
>
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> ---
> git-compat-util.h | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> Another possibility would be to add a "const char *default"
> argument to the function, and to do:
>
> if (!*p) {
> *val = default;
> return 1;
> }
>
> This could make the function more useful in some cases.
>
> I also wonder if the function is too big to be inlined, and
> in that case, in which file it should be added.
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cedad4d581..7ee040388f 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -534,6 +534,41 @@ static inline int ends_with(const char *str, const char *suffix)
> return strip_suffix(str, suffix, &len);
> }
>
> +/*
> + * If the string "str" is the same as the string in "prefix", then the "val"
> + * parameter is set to the empty string and 1 is returned.
> + * If the string "str" begins with the string found in "prefix" and then a
> + * "=" sign, then the "val" parameter is set to "str + strlen(prefix) + 1"
> + * (i.e., to the point in the string right after the prefix and the "=" sign),
> + * and 1 is returned.
> + *
> + * Otherwise, return 0 and leave "val" untouched.
> + *
> + * When we accept both a "--key" and a "--key=<val>" option, this function
> + * can be used instead of !strcmp(arg, "--key") and then
> + * skip_prefix(arg, "--key=", &arg) to parse such an option.
> + */
> +static inline int skip_to_opt_val(const char *str, const char *prefix,
> + const char **val)
> +{
> + const char *p;
> +
> + if (!skip_prefix(str, prefix, &p))
> + return 0;
> +
> + if (!*p) {
> + *val = "";
> + return 1;
> + }
> +
> + if (*p == '=') {
> + *val = p + 1;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> #define SWAP(a, b) do { \
> void *_swap_a_ptr = &(a); \
> void *_swap_b_ptr = &(b); \
