[PATCH v1] Added string conversion utility functions

[Bill Spitzak]

On 11/24/2014 05:08 AM, Imran Zaman wrote:
> Thanks Bill for your comments. Please see my comments inline.
>
> On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak <spitzak at gmail.com> wrote:
>> There is no need to pass endptr, because your functions will always fail if
>> it is not set to point to the terminating null.
>>
> [IZ] endptr is passed other than null; please see the whole patch. why
> do u think it will fail?

What I meant was that if this function returns success, endptr will 
ALWAYS be set to the string+strlen(string), ie at the NULL. As the 
caller can easily create this answer itself it seems useless to pass a 
pointer to store it.

Although endptr might not point at the null when this fails, it seems 
hard for the caller to do anything useful with this. It would have to 
first determine if the error was due to something other than trailing 
characters, which would involve replicating the entire function locally.

Therefore I think this argument is useless and there is no reason to 
support it.

Also the grep below shows that nobody uses the endptr except to do the 
error checking that this function is removing the need for.

>> I also suspect that the base is never used. In addition a possible future
>> fix would be to accept 0x for hex but a leading zero is decimal, not octal,
>> which means this function would generate the base.
>>
> [IZ] Base is used in weston code as well, please see the whole patch.

Out of 14 calls (found with "git grep strto"), 6 use 10, 1 uses 16, and 
7 use 0.

I believe all the "10"s are to avoid the unexpected-octal problem. I 
can't be absolutely certain, but I suspect the reason this is done is to 
prevent unexpected octal conversion, and in fact there is no desire to 
make 0x prefix not work.

This is a good indication that perhaps your functions should do this as 
well. Basically only base 10 and 0x for base 16 work.

wscreensaver-glue uses 16 to read the color map out of an xpm file. I 
cannot find any code in weston that calls this xpm converter.

>> I am also worried about passing the out ptr. Depending on the sizes of int,
>> long, and long long this will easily result in code that compiles on one
>> platform but not on another. Returning the value would avoid this and reduce
>> the number of functions. Instead pass a pointer to a variable to store the
>> error into, or use errno?
>>
>> Would prefer you not test the addresses of out parameters for null. It
>> should crash at this point, as this is a programming error, not a runtime
>> error. Returning false will make it hard to figure out what is wrong.
>>
> [IZ] can u please elaborate it bit more as I can add more test cases
> to cover the corner case if I understand clearly what do you want to
> highlight?

I want it it CRASH if null is passed for the output pointer. The easiest 
way to do this is to not check if it is null.

Please do not return zero or do anything other than crash right at that 
point. This is not an environment or input error, passing null for this 
is a programming error that can be fixed at compile time. Hiding the 
error so it is not obvious the first time the program is run is 
completely non-productive.

>> It is useful to pass -1 for strtoul and this should continue to work. It is
>> the only easily-recognized value for "largest number possible". May be ok to
>> not allow other negative numbers however.

Yes I don't think preserving -1 is a big deal, but I have used this in 
the past. The caller can certainly check for this string before calling 
the converter.