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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 17, 2015, 19:46 UTC
Message-ID: <550884B3.5010102@alum.mit.edu>
In-Reply-To: <xmqqk2yfo3y0.fsf@gitster.dls.corp.google.com>

On 03/17/2015 07:48 PM, Junio C Hamano wrote:

Show 16 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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.
>>
>> git-compat-util.h has two functions, strtoul_ui() and strtol_i(), that
>> try to make parsing integers a little bit easier.
> 
> Exactly.  They were introduced to prevent sloppy callers from
> passing NULL to the &end parameter to underlying strtoul(3).  The
> first thing that came to my mind while reading your message up to
> this point was "why not use them more, tighten them, adding
> different variants if necessary, instead of introducing an entirely
> new set of functions in a new namespace?"

Here were my thoughts:

* I wanted to change the interface to look less like
  strtol()/strtoul(), so it seemed appropriate to make the names
  more dissimilar.

* The functions seemed long enough that they shouldn't be inline,
  and I wanted to put them in their own module rather than put
  them in git-compat-util.h.

* It wasn't obvious to me how to generalize the names, strtoul_ui()
  and strtol_i(), to the eight functions that I wanted.

That being said, I'm not married to the names. Suggestions are welcome--but we would need names for 8 functions, not 4 [1].

Michael

Show 19 quoted lines

> For example:
> 
>> * Am I making callers too strict? In many cases where a positive
>>   integer is expected (e.g., "--abbrev=<num>"), I have been replacing
>>   code like
>>
>>       result = strtoul(s, NULL, 10);
>>
>>   with
>>
>>       if (convert_i(s, 10, &result))
>>               die("...");
> 
> I think strictness would be good and those who did "--abbrev='  20'"
> deserve what they get from the additional strictness, but 
> 
> 	if (strtol_i(s, 10, &result))
> 
> would have been more familiar.

[1] It could be that when we're done, it will turn out that some of the eight variants are not needed. If so, we can delete them then.

-- 
Michael Haggerty
mhagger@alum.mit.edu