Re: [PATCH] remove svn__strtol() and svn__strtoul()

On 6 August 2014 19:57, Stefan Sperling <stsp_at_elego.de> wrote:
> On Wed, Aug 06, 2014 at 07:31:04PM +0400, Ivan Zhakov wrote:
>> apr_get_os_error() uses GetLastError() on Windows, while strtol() uses
>> standard errno for error code.
>
> So you're saying code like this works on Windows, and I shouldn't
> be using apr_get/set_os_error() for strtol()?
>
> errno = 0;
> lval = strtol(buf, &ep, 10);
> if (buf[0] == '\0' || *ep != '\0')
> goto not_a_number;
> if ((errno == ERANGE && (lval == LONG_MAX || lval == LONG_MIN)) ||
> (lval > INT_MAX || lval < INT_MIN))
> goto out_of_range;
> ival = lval;
Yes, it should work. Actually you shouldn't use apr_get_os_error() for
non-Windows platforms also. This is not Windows specific problem. Btw
svn_cstring_strtoi64() implementation already uses errno, not
apr_get_os_error():
[[[
  /* We assume errno is thread-safe. */
  errno = 0; /* APR-0.9 doesn't always set errno */

  val = apr_strtoi64(str, &endptr, base);
  if (errno == EINVAL || endptr == str || str[0] == '\0' || *endptr != '\0')
    return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
                             _("Could not convert '%s' into a number"),
                             str);
  if ((errno == ERANGE && (val == APR_INT64_MIN || val == APR_INT64_MAX)) ||
      val < minval || val > maxval)
    /* ### Mark this for translation when gettext doesn't choke on macros. */
    return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
                             "Number '%s' is out of range "
                             "'[%" APR_INT64_T_FMT ", %" APR_INT64_T_FMT "]'",
                             str, minval, maxval);

]]]

Bert pointed that Microsoft CRT implementation of stroul() is slow due
locales handling. I'm fine to have our own implementation of strtoul()
in this, but with appropriate error handling and *without* any
micro-optimisation that makes code harder to understand and review.

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com