Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2015, 06:22 UTC
Message-ID: <xmqq7fudld61.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1426608016-2978-1-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> * It allows leading whitespace.

This might be blessing in disguise. Our friends on MacOS may be relying on that

    git cmd --abbrev="$(wc -c <foo)"

to work "as expected", even though their "wc" gives leading spaces, for example.

> * It allows arbitrary trailing characters.

Which is why we have introduced strtoul_ui() and such.

> * It allows a leading sign character ('+' or '-'), even though the
>   result is unsigned.

I do not particularly see it a bad thing to accept "--abbrev=+7". Using unsigned type to accept a width and parsing "--abbrev=-7" into a large positive integer _is_ a problem, and we may want to fix it, but I think that is still within the scope of the original "better strtol/strtoul", and I do not see it as a justification to introduce a set of functions with completely unfamiliar name.

> * If the string doesn't contain a number at all, it sets its "endptr"
>   argument to point at the start of the string but doesn't set errno.

Why is that a problem? A caller that cares is supposed to check endptr and the string it gave, no? Now, if strtoul_ui() and friends do not do so, I again think that is still within the scope of the original "better strtol/strtoul".

> * If the value is larger than fits in an unsigned long, it returns the
>   value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).

Ditto.

> * If the value is between -ULONG_MAX and 0, it returns the positive
>   integer with the same bit pattern, without setting errno(!) (I can
>   hardly think of a scenario where this would be useful.)

Ditto.

> * For base=0 (autodetect base), it allows a base specifier prefix "0x"
>   or "0" and parses the number accordingly. For base=16 it also allows
>   a "0x" prefix.

Why is it a problem to allow "git cmd --hexval=0x1234", even if "git cmd --hexval=1234" would suffice?

> When I looked around, I found scores of sites that call atoi(),
> strtoul(), and strtol() carelessly. And it's understandable. Calling
> any of these functions safely is so much work as to be completely
> impractical in day-to-day code.

Yes, these burdens on the caller were exactly why strtoul_ui() etc. wanted to reduce---and it will be a welcome change to do necessary checks that are currently not done.

Show 10 quoted lines

> Please see the docstrings in numparse.h in the first commit for
> detailed API docs. Briefly, there are eight new functions:
>
>     parse_{l,ul,i,ui}(const char *s, unsigned int flags,
>                       T *result, char **endptr);
>     convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);
>
> The parse_*() functions are for parsing a number from the start of a
> string; the convert_*() functions are for parsing a string that
> consists of a single number.

I am not sure if I follow. Why not unify them into one 4-function set, with optional endptr that could be NULL?

While we are on the topic of improving number parsing, one thing that I found somewhat frustrating is that config.c layer knows the scaling suffix but option parsing layer does not. These functions should offer an option (via their "flags") to say "I allow scaled numbers like 2k, 4M, etc.".