Re: [PATCH v2 4/5] parse-options: introduce `OPTION_UNSIGNED`

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 16, 2025, 10:27 UTC
Message-ID: <Z_-GKX8uFSNib5rW@pks.im>
In-Reply-To: <35300426-32cc-4c0e-b0c3-edb2b2ed312f@gmail.com>

On Tue, Apr 15, 2025 at 04:52:02PM +0100, Phillip Wood wrote:

Show 20 quoted lines

> Hi Patrick
> 
> On 15/04/2025 13:14, Patrick Steinhardt wrote:
> > We have two generic ways to parse integers in the "parse-options"
> > subsytem:
> > 
> >    - `OPTION_INTEGER` parses a signed integer.
> > 
> >    - `OPTION_MAGNITUDE` parses an unsigned integer, but it also
> >      interprets suffixes like "k" or "g".
> > 
> > Notably missing is a middle ground that parses unsigned integers without
> > interpreting suffixes. Introduce a new `OPTION_UNSIGNED` option type to
> > plug this gap. This option type will be used in subsequent commits.
> 
> I think this is a useful addition. I wonder about the way it handles
> negative values though. For types narrower than uintmax_t "-1" will be
> rejected but large negative values that parse as small positive numbers will
> be accepted. Perhaps we should explicitly reject strings starting with "-"
> as we do in git_parse_ulong() which is used by OPTION_MAGNITUDE.

Wait, does it? Why would `strtoul()` or any of its variants ever accept a string prefixed with a "-"?

    If the minus sign was part of the input sequence, the numeric value
    calculated from the sequence of digits is negated as if by unary
    minus in the result type, which applies unsigned integer wraparound
    rules.

Oh dear... all these integer conversion functions are really a gift that keeps on giving. Gross.

> This patch also needs the fix from patch 2 to detect overflows for
> uintmax_t.

Yup, will add.

Patrick