On Wed, Sep 15, 2010 at 02:14:36PM +0100, Julian Foad wrote:
> On Tue, 2010-09-07, stsp_at_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.)
Will fix that.
> > 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.
It's C-string, i.e. NUL-terminated. The number format is the same as
apr_strtoi64() expects, which is equivalent to the C standard function
strtol().
> > + * @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.
I'll just change that to say "integer". Internally, it uses an int,
and assumes that any value from APR_INT32_MIN to APR_INT32_MAX will fit.
> > 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?
Forward planning :)
As soon as apr_strtoui64() exists, we can switch this function to use
it without much effort.
> > + 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.
I'd prefer to keep it for clarity until this function can work with
unsigned integers only. Note also a later commit which forced the minval
and maxval comparisons to be signed.
> > + 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.
Why is it inconsistent?
> > + *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.
It's much better in HEAD :)
> > +
> > +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.
Again, why inconsistent?
Thanks for the review! Please look at later commits to this code also
because a lot has happened since the initial commit.
Stefan
Received on 2010-09-15 17:10:53 CEST