[PATCH v1] Added string conversion utility functions

[Imran Zaman]

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?

> 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);
>> +}
>
>