Re: [PATCH 01/14] numparse: new module for parsing integral numbers
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 20, 2015, 17:51 UTC
- Message-ID
- <xmqq7fubftfk.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1426608016-2978-2-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 44 quoted lines
> +static int parse_precheck(const char *s, unsigned int *flags)
> +{
> + const char *number;
> +
> + if (isspace(*s)) {
> + if (!(*flags & NUM_LEADING_WHITESPACE))
> + return -NUM_LEADING_WHITESPACE;
> + do {
> + s++;
> + } while (isspace(*s));
> + }
> +
> + if (*s == '+') {
> + if (!(*flags & NUM_PLUS))
> + return -NUM_PLUS;
> + number = s + 1;
> + *flags &= ~NUM_NEGATIVE;
> + } else if (*s == '-') {
> + if (!(*flags & NUM_MINUS))
> + return -NUM_MINUS;
> + number = s + 1;
> + *flags |= NUM_NEGATIVE;
> + } else {
> + number = s;
> + *flags &= ~NUM_NEGATIVE;
> + }
> +
> + if (!(*flags & NUM_BASE_SPECIFIER)) {
> + int base = *flags & NUM_BASE_MASK;
> + if (base == 0) {
> + /* This is a pointless combination of options. */
> + die("BUG: base=0 specified without NUM_BASE_SPECIFIER");
> + } else if (base == 16 && starts_with(number, "0x")) {
> + /*
> + * We want to treat this as zero terminated by
> + * an 'x', whereas strtol()/strtoul() would
> + * silently eat the "0x". We accomplish this
> + * by treating it as a base 10 number:
> + */
> + *flags = (*flags & ~NUM_BASE_MASK) | 10;
> + }
> + }
> + return 0;
> +}I somehow feel that a pre-processing that only _inspects_ part of the string, without munging that string (e.g. notice '-' but feed that to underlying strtol(3)) somewhat a brittle approach. When I read the above for the first time, I somehow expected that the code would notice leading '-', strip that leading '-' and remember the fact that it did so in the *flags, let the strtol(3) to parse the remainder _and_ always make sure the returned result is not negative (because that would imply that the original input had two leading minuses and digits), and give the sign based on what this preprocess found out in *flags, and then seeing that there is no sign of such processing in the caller I scratched my head.
I still have not convinced myself that what I am seeing in the base==16 part in the above is correct.