Re: [bugs] [PATCH] Fix integer parsing

On Thu, Nov 30, 2017 at 06:46:02PM +0100, Lukas Fleischer wrote:
> On Wed, 29 Nov 2017 at 09:13:08, Lars Henriksen wrote:
> > On Tue, Nov 28, 2017 at 08:50:35PM +0100, Lukas Fleischer wrote:
> > > On Tue, 28 Nov 2017 at 20:32:26, Lars Henriksen wrote:
> > > >  static int config_parse_int(int *dest, const char *val)
> > > >  {
> > > > -       if ((*val == '+' || *val == '-' || isdigit(*val))
> > > > -           && is_all_digit(val + 1))
> > > 
> > > If the value is a single digit, isdigit(*val) is true and is_all_digit()
> > > is invoked with an empty string (which should also return true). So
> > > everything is as it should be...?
> > 
> > config_parse_int(&dest, "+")?
> > 
> 
> Ah, I see. I guess this should be something like
> 
>     if (*cp == '+' || *cp == '-')
>         cp++;
>     if (isdigit(*cp) && is_all_digit(cp + 1))
>         [...]
> 
> then, right?

Well, yes, if you insist that is_all_digit() should return true on an empty
string, but I am not convinced. You wrote:

> > > The semantics should match the name of the function which roughly asks
> > > "Is each of the characters in the string a digit?" or, equivalently,
> > > "Does the input *not* contain any character which is *not* a digit?"

> > > And, quite clearly, an empty string does not contain any character which
> > > is not a digit, so the function should return true.

A member of the empty set has any property you may care to ask about. That's
true, but as interesting as 1 = 1. The logic is impeccable, but is it useful?
In a way you have supplied the answer yourself.

Let's forget the +/- issue which is not the point. You have a string which you
would like to interpret as an integer. Can you just give it to is_all_digit()?
Not with your semantics, because if it returns true, all you know is that it
does not contain a character that is not a digit. So you check whether the
first character is a digit and leave the rest to is_all_digit(). That is odd.
The first character is checked "by hand", the rest is left to a supporting
function.

The reason is your semantics. Mine are something like "Does this string
contain some digits and only digits?" This is not about logic, but about
usefulness, and I would make the existence (non-empty) check built-in.
But if it is not, you need only check non-emptyness before the call:

if (*cp && is_all_digit(cp))
	[...]

Lars Henriksen

Links