Thanks Bill for your comments. Please see my comments inline.
On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak <spit...@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?
> 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.
> 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?
> 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.
>
[IZ] IMO its good to keep the APIs simple and homogeneous with
appropriate data structures needed for the input
BR
imran
> On 11/21/2014 07:38 AM, Imran Zaman wrote:
>
>> +static bool
>> +convert_strtol(const char *str, char **endptr, int base, long *val)
>> +{
>> + char *end = NULL;
>> + long v;
>> + int prev_errno = errno;
>> +
>
>
>> + if (!str || !val)
>> + return false;
>
> Please do not do the above tests!
>
>> + if (!endptr)
>> + endptr = &end;
>> +
>> + errno = 0;
>> + v = strtol(str, endptr, base);
>> + if (errno != 0 || *endptr == str || **endptr != '\0')
>> + return false;
>> +
>> + errno = prev_errno;
>> + *val = v;
>> + return true;
>> +}
>> +
>> +static bool
>> +convert_strtoul (const char *str, char **endptr, int base, unsigned long
>> *val)
>> +{
>> + char *end = NULL;
>> + unsigned long v;
>> + int i = 0;
>> + int prev_errno = errno;
>> +
>> + if (!str || !val)
>> + return false;
>
>
>> + /* check for negative numbers */
>> + while (str[i]) {
>> + if (!isspace(str[i])) {
>> + if (str[i] == '-')
>> + return false;
>> + else
>> + break;
>> + }
>> + i++;
>> + }
>
> You could do the above test afterwards and check for and allow the -1 value.
>
>
>> +
>> + if (!endptr)
>> + endptr = &end;
>> +
>> + errno = 0;
>> + v = strtoul(str, endptr, base);
>> + if (errno != 0 || *endptr == str || **endptr != '\0')
>> + return false;
>> +
>> + errno = prev_errno;
>> + *val = v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoi(const char *str, char **endptr, int base, int *val)
>> +{
>> + long v;
>> +
>> + if (!convert_strtol(str, endptr, base, &v) || v > INT_MAX
>> + || v < INT_MIN)
>> + return false;
>> +
>> + *val = (int)v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtol(const char *str, char **endptr, int base, long *val)
>> +{
>> + return convert_strtol(str, endptr, base, val);
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoui(const char *str, char **endptr, int base, unsigned int
>> *val)
>> +{
>> + unsigned long v;
>> +
>> + if (!convert_strtoul(str, endptr, base, &v) || v > UINT_MAX)
>> + return false;
>> +
>> + *val = (unsigned int)v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoul(const char *str, char **endptr, int base, unsigned long
>> *val)
>> +{
>> + return convert_strtoul(str, endptr, base, val);
>> +}
>
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
