git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 20, 2015, 08:54 UTC
Message-ID
<CAPig+cTht9bCoJx3dCj3m_z59zp0C-PdzUYJVbZ7kO5ryNaOQQ@mail.gmail.com>
In-Reply-To
<550A008B.3050900@alum.mit.edu>
On Wed, Mar 18, 2015 at 6:47 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 33 quoted lines
> On 03/18/2015 07:27 PM, Eric Sunshine wrote:
>> On Tuesday, March 17, 2015, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> Implement wrappers for strtol() and strtoul() that are safer and more
>>> convenient to use.
>>> + * The lowest 6 bits of flags hold the numerical base that should be
>>> + * used to parse the number, 2 <= base <= 36. If base is set to 0,
>>> + * then NUM_BASE_SPECIFIER must be set too; in this case, the base is
>>> + * detected automatically from the string's prefix.
>>
>> Does this restriction go against the goal of making these functions
>> convenient, even while remaining strict? Is there a strong reason for
>> not merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would
>> make it consistent with strto*l() without (I think) introducing any
>> ambiguity.
>
> I thought about doing this. If it were possible to eliminate
> NUM_BASE_SPECIFIER altogether, then there is no doubt that it would be a
> good change. But NUM_BASE_SPECIFIER also has an effect when base==16;
> namely, that an "0x" prefix, if present, is consumed. So
>
>     parse_i("0xff", 16 | NUM_BASE_SPECIFIER, &result, &endptr)
>
> gives result==255 and endptr==s+4, whereas
>
>     parse_i("0xff", 16, &result, &endptr)
>
> gives result==0 and entptr==s+1 (it treats the "x" as the end of the
> string).
>
> We could forgo that feature, effectively allowing a base specifier if
> and only if base==0. But I didn't want to rule out allowing an optional
> base specifier for base==16, in which case NUM_BASE_SPECIFIER can't be
> dispensed with entirely.

Making base==0 a special case doesn't have to mean ruling out or eliminating NUM_BASE_SPECIFIER for the base==16 case. However, a special case base==0 would make the API a bit non-orthogonal and require extra documentation. But for those familiar with how strto*l() treats base==0 specially, the rule of least surprise may apply.

> If you agree with that, then the remaining question is: which policy is
> less error-prone? My thinking was that forcing the caller to specify
> NUM_BASE_SPECIFIER explicitly when they select base==0 will serve as a
> reminder that the two features are intertwined.

Since base==0 would unambiguously imply NUM_BASE_SPECIFIER, being able to tersely say convert_i(s, 0, &result) would be a win from a convenience perspective. However...

Show 10 quoted lines
> Because another
> imaginable policy (arguably more consistent with the policy for base!=0)
> would be that
>
>     convert_i(s, 0, &result)
>
> , because it *doesn't* specify NUM_BASE_SPECIFIER, doesn't allow a base
> prefix, and therefore indirectly only allows base-10 numbers.
>
> But I don't feel strongly about this.

I don't feel strongly about it either, and can formulate arguments either way. Assuming you stick with your current design, if the strictness of having to specify NUM_BASE_SPECIFIER for base==0 proves too burdensome, it can be loosened later by making base==0 a special case.

On the other hand, if you go with Junio's suggestion of choosing names for these functions which more closely mirror strto*l() names, then the rule of least surprise might suggest that a special case for base==0 has merit.

Show 26 quoted lines
>>> + * Return 0 on success or a negative value if there was an error. On
>>> + * failure, *result and *entptr are left unchanged.
>>> + *
>>> + * Please note that if NUM_TRAILING is not set, then it is
>>> + * nevertheless an error if there are any characters between the end
>>> + * of the number and the end of the string.
>>
>> Again, on the subject of convenience, why this restriction? The stated
>> purpose of the parse_*() functions is to parse the number from the
>> front of the string and return a pointer to the first non-numeric
>> character following. As  a reader of this API, I interpret that as
>> meaning that NUM_TRAILING is implied. Is there a strong reason for not
>> inferring NUM_TRAILING for the parse_*() functions at the API level?
>> (I realize that the convert_*() functions are built atop parse_*(),
>> but that's an implementation detail.)
>
> Yes, I'd also thought about that change:
>
> * Make NUM_TRAILING private.
> * Make the current parse_*() functions private.
> * Add new parse_*(s, flags, result, endptr) functions that imply
> NUM_TRAILING (and maybe they should *require* a non-null endptr argument).
> * Change the convert_*() to not allow the NUM_TRAILING flag.
>
> This would add a little bit of code, so I didn't do it originally. But
> since you seem to like the idea too, I guess I will make the change.

The other option (as Junio also suggested) is to collapse this API into just the four parse_*() functions. All of the call-sites you converted in this series invoked convert_*(), but it's not clear that having two sets of functions which do almost the same thing is much of a win. If the default is for NUM_TRAILING to be off, and if NULL 'endptr' is allowed, then:

    parse_ul(s, 10, &result, NULL);
doesn't seem particularly burdensome compared with:
    convert_ul(s, 10, &result);

A more compact API, with only the parse_*() functions, means less to remember and less to document, and often is more orthogonal.

The argument for dropping the convert_*() functions is strengthened if you follow Junio's suggestion to name these functions after the strto*l() variations.

Show 13 quoted lines
>>> +int parse_l(const char *s, unsigned int flags,
>>> +           long *result, char **endptr);
>>
>> Do we want to perpetuate the ugly (char **) convention for 'endptr'
>> from strto*l()? Considering that the incoming string is const, it
>> seems undesirable to return a non-const pointer to some place inside
>> that string.
>
> Yes, I guess we could do some internal casting and expose endptr as
> (const char **). It would make it a bit harder if the user actually
> wants to pass a non-const string to the function and then modify the
> string via the returned endptr, but I haven't run into that pattern yet
> so let's make the change you suggested.

I don't feel strongly about this either. It always struck me as a bit of an API wart, but I can see the convenience value of non-const 'endptr' if the caller intends to modify the string. If you rename these functions to mirror strto*l(), then the rule of least surprise would suggest that 'endptr' should remain non-const.

Previous: Michael HaggertyNext: Junio C Hamano
Message 5 of 43 in “numparse module: systematically tighten up integer parsing”
  1. 00/14 numparse module: systematically tighten up integer parsingMichael Haggerty, Mar 17, 2015
  2. 01/14 numparse: new module for parsing integral numbersMichael Haggerty, Mar 17, 2015
  3. Eric SunshineMar 18, 2015
  4. Michael HaggertyMar 18, 2015
  5. Eric SunshineMar 20, 2015
  6. Junio C HamanoMar 20, 2015
  7. 02/14 cacheinfo_callback(): use convert_ui() when handling "--cacheinfo"Michael Haggerty, Mar 17, 2015
  8. 03/14 write_subdirectory(): use convert_ui() for parsing modeMichael Haggerty, Mar 17, 2015
  9. 04/14 handle_revision_opt(): use skip_prefix() in many placesMichael Haggerty, Mar 17, 2015
  10. 05/14 handle_revision_opt(): use convert_i() when handling "-<digit>"Michael Haggerty, Mar 17, 2015
  11. Junio C HamanoMar 19, 2015
  12. 06/14 strtoul_ui(), strtol_i(): remove functionsMichael Haggerty, Mar 17, 2015
  13. 07/14 handle_revision_opt(): use convert_ui() when handling "--abbrev="Michael Haggerty, Mar 17, 2015
  14. 08/14 builtin_diff(): detect errors when parsing --unified argumentMichael Haggerty, Mar 17, 2015
  15. 09/14 opt_arg(): val is always non-NULLMichael Haggerty, Mar 17, 2015
  16. 10/14 opt_arg(): use convert_i() in implementationMichael Haggerty, Mar 17, 2015
  17. 11/14 opt_arg(): report errors parsing option valuesMichael Haggerty, Mar 17, 2015
  18. 12/14 opt_arg(): simplify pointer handlingMichael Haggerty, Mar 17, 2015
  19. 13/14 diff_opt_parse(): use convert_i() when handling "-l<num>"Michael Haggerty, Mar 17, 2015
  20. 14/14 diff_opt_parse(): use convert_i() when handling --abbrev=<num>Michael Haggerty, Mar 17, 2015
  21. Junio C HamanoMar 19, 2015
  22. Junio C HamanoMar 17, 2015
  23. Michael HaggertyMar 17, 2015
  24. Junio C HamanoMar 19, 2015
  25. Duy NguyenMar 17, 2015
  26. Michael HaggertyMar 18, 2015
  27. Duy NguyenMar 18, 2015
  28. Jeff KingMar 18, 2015
  29. Michael HaggertyMar 18, 2015
  30. Jeff KingMar 19, 2015
  31. Junio C HamanoMar 19, 2015
  32. Junio C HamanoMar 19, 2015
  33. Michael HaggertyMar 24, 2015
  34. René ScharfeMar 24, 2015
  35. Michael HaggertyMar 25, 2015
  36. Junio C HamanoMar 25, 2015
  37. Michael HaggertyMar 24, 2015
  38. Junio C HamanoMar 19, 2015
  39. Michael HaggertyMar 24, 2015
  40. Junio C HamanoMar 24, 2015
  41. Junio C HamanoMar 24, 2015
  42. Michael HaggertyMar 24, 2015
  43. Junio C HamanoMar 24, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.