Re: [PATCH 01/14] numparse: new module for parsing integral numbers

[Junio C Hamano <gitster@pobox.com>]

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

> +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.