On Tue, 2010-09-07, s...@apache.org wrote:
> Author: stsp
> Date: Tue Sep 7 00:09:46 2010
> New Revision: 993183
>
> URL: http://svn.apache.org/viewvc?rev=993183&view=rev
> Log:
> Introduce a new family of functions to parse numbers from strings.
Looks useful. Thanks.
> The goal is to replace direct calls to functions like atoi() and apr_atoi64(),
> which have very inconvenient error handling to the point where a lot of
> our code simply ignores conversion errors and continues operating with
> the converted data in potentially undefined state.
> The new functions, on the other hand, report conversion errors via the
> standard Subversion error handling interface, thereby not allowing callers
> to ignore conversion errors.
>
> This commit also switches selected pieces of code over to the new functions.
> More conversion to come.
>
> * subversion/include/svn_string.h
> (svn_cstring_strtoi64, svn_cstring_atoi64, svn_cstring_atoi,
> svn_cstring_strtoui64, svn_cstring_atoui64, svn_cstring_atoui): Declare.
>
> * subversion/libsvn_subr/svn_string.c
> (): Include svn_private_config.h for the _() gettext macro.
> (svn_cstring_strtoi64, svn_cstring_strtoui64, svn_cstring_atoi64,
> svn_cstring_atoi): New.
And the other two functions as well? (Just missing from the log
message.)
> * subversion/libsvn_ra_serf/serf.c
> (dirent_walker): Use svn_cstring_atoi64() instead of apr_atoi64().
>
> * subversion/libsvn_diff/parse-diff.c
> (parse_offset): Call svn_cstring_strtoui64() instead of calling
> apr_atoi64() and performing manual overflow checking.
>
> * subversion/mod_dav_svn/reports/log.c
> (dav_svn__log_report): Use svn_cstring_atoi() instead of atoi() for
> parsing CDATA of the "limit" element.
>
> * subversion/mod_dav_svn/reports/replay.c
> (dav_svn__replay_report): Use svn_cstring_strtoi64() instead of atoi() for
> parsing CDATA of the "send-deltas" element.
>
> * subversion/libsvn_subr/win32_xlate.c
> (get_page_id_from_name): Use svn_cstring_atoui() instead of atoi() to parse
> the number of a codepage.
>
> * subversion/libsvn_subr/io.c
> (svn_io_read_version_file): Use svn_cstring_atoi() instead of atoi().
>
> * subversion/libsvn_subr/hash.c
> (svn_hash_read): Use svn_cstring_atoi() instead of atoi().
> Modified: subversion/trunk/subversion/include/svn_string.h
> +/**
> + * Parse the C string @a str into a 64 bit number, and return it in @a *n.
> + * Assume that the number is represented in base @a base.
> + * Raise an error if conversion fails (e.g. due to overflow), or if the
> + * converted number is smaller than @a minval or larger than @a maxval.
What are the semantics regarding 'str' - does it ignore leading spaces,
a plus sign, decimal point, trailing spaces, trailing non-spaces?
Preferably refer to a standard function to make this clear without
trying to spell it all out in full.
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_strtoi64(apr_int64_t *n, const char *str,
> + apr_int64_t minval, apr_int64_t maxval,
> + int base);
> +
> +/**
> + * Parse the C string @a str into a 64 bit number, and return it in @a *n.
> + * Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoi64(apr_int64_t *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into a 32 bit number, and return it in @a *n.
What does "parse into a 32 bit number" mean when 'int' is not 32 bits?
If the intention is to limit this to 32-bit numbers its name should
reflect it. (The data type *could* stay as 'int' because we are allowed
to assume it's at least 32 bits, but I don't know if we'd want to do
that.)
Otherwise it should support the range of 'int' and not assume 32 bits.
> + * Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoi(int *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into an unsigned 64 bit number, and return
> + * it in @a *n. Assume that the number is represented in base @a base.
> + * Raise an error if conversion fails (e.g. due to overflow), or if the
> + * converted number is smaller than @a minval or larger than @a maxval.
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_strtoui64(apr_uint64_t *n, const char *str,
> + apr_uint64_t minval, apr_uint64_t maxval,
> + int base);
> +
> +/**
> + * Parse the C string @a str into an unsigned 64 bit number, and return
> + * it in @a *n. Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoui64(apr_uint64_t *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into an unsigned 32 bit number, and return
> + * it in @a *n. Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoui(unsigned int *n, const char *str);
[...]
> Modified: subversion/trunk/subversion/libsvn_subr/svn_string.c
> +svn_error_t *
> +svn_cstring_strtoui64(apr_uint64_t *n, const char *str,
> + apr_uint64_t minval, apr_uint64_t maxval,
> + int base)
> +{
> + apr_int64_t parsed;
> +
> + /* We assume errno is thread-safe. */
> + errno = 0; /* APR-0.9 doesn't always set errno */
> +
> + /* ### We're throwing away half the number range here.
> + * ### Maybe implement our own number parser? */
What use is this function, compared to _strtoi64(), if it doesn't
actually support the top half of the range?
> + parsed = apr_strtoi64(str, NULL, base);
> + if (errno != 0)
> + return svn_error_return(
> + svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Could not convert '%s' into a number"),
> + str));
> + if (parsed < 0 || parsed < minval || parsed > maxval)
Forget "parsed < 0"; the "parsed < minval" term will always catch that
case because minval can't be negative.
> + return svn_error_return(
> + svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Number '%s' is out of range"), str));
> + *n = parsed;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cstring_atoui64(apr_uint64_t *n, const char *str)
> +{
> + return svn_error_return(svn_cstring_strtoui64(n, str, 0,
> + APR_UINT64_MAX, 10));
> +}
> +
> +svn_error_t *
> +svn_cstring_atoui(unsigned int *n, const char *str)
> +{
> + apr_uint64_t parsed;
> +
> + SVN_ERR(svn_cstring_strtoui64(&parsed, str, 0, APR_UINT32_MAX, 10));
Inconsistent range limits - see above.
> + *n = (unsigned int)parsed;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cstring_strtoi64(apr_int64_t *n, const char *str,
> + apr_int64_t minval, apr_int64_t maxval,
> + int base)
> +{
> + apr_int64_t parsed;
> +
> + /* We assume errno is thread-safe. */
> + errno = 0; /* APR-0.9 doesn't always set errno */
> +
> + parsed = apr_strtoi64(str, NULL, base);
> + if (errno != 0)
> + return svn_error_return(
> + svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Could not convert '%s' into a number"),
> + str));
> + if (parsed < minval || parsed > maxval)
> + return svn_error_return(
> + svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Number '%s' is out of range"), str));
> + *n = parsed;
> + return SVN_NO_ERROR;
> +}
Nice.
> +
> +svn_error_t *
> +svn_cstring_atoi64(apr_int64_t *n, const char *str)
> +{
> + return svn_error_return(svn_cstring_strtoi64(n, str, APR_INT64_MIN,
> + APR_INT64_MAX, 10));
> +}
> +
> +svn_error_t *
> +svn_cstring_atoi(int *n, const char *str)
> +{
> + apr_int64_t parsed;
> +
> + SVN_ERR(svn_cstring_strtoi64(&parsed, str, APR_INT32_MIN,
> + APR_INT32_MAX, 10));
Inconsistent range limits - see above.
> + *n = (int)parsed;
> + return SVN_NO_ERROR;
> +}
- Julian
