Re: [PATCH 1/3] git-compat-util: introduce skip_to_opt_val()
Christian Couder <christian.couder@gmail.com> writes:
Show 17 quoted lines
> From: Christian Couder <christian.couder@gmail.com>
>
> 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.
Show 65 quoted lines
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> 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); \
Christian Couder
