[PATCH v1] Added string conversion utility functions

[Imran Zaman]

Hi

Please see the comments below.

BR
imran

On Mon, Nov 24, 2014 at 8:58 PM, Bill Spitzak <spitzak at gmail.com> wrote:
>
>
> 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.
>

[IZ2] This is the case where endptr is used in patch. I can remove
endptr but it seems its used so i have to keep the existing
functionality in place.
+ if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10)


>>> 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.
>
[IZ2] Sorry still not able to grasp what you want to highlight. Can
you please give an example?

>>> 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.

BR
imran