Stefan Sperling wrote:
> On Wed, Sep 15, 2010 at 02:14:36PM +0100, Julian Foad wrote:
> > On Tue, 2010-09-07, stsp_at_apache.org wrote:
> > > * 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().
Great, either of those will make a good reference from the doc string.
> > > + * @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 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".
Great.
> Internally, it uses an int,
> and assumes that any value from APR_INT32_MIN to APR_INT32_MAX will fit.
That's the inconsistency I mentioned below: it claims to support 'int'
but the implementation enforces a 32-bit range limit regardless of the
size of 'int'. It should enforce the range of the actual 'int' type
instead. (If the code uses 64-bit internally and assumes that 'int' is
<= 64 bits, I'd be OK with that.)
> > > 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.
OK, that's fine if we're only using it to read counts of real things,
like real file sizes. If we want to read arbitrary 64-bit numbers such
as digests or virtual memory addresses, then we might need the upper
range to work.
What's the failure mode? From a quick look at the present code I think
it will return an error like "Number '(2^63)' is out of range '[0, (2^64
- 1)]'" which is not exactly correct but at least it leads the developer
to the right part of the source code.
Please add a note in the doc string that these two functions only
support numbers up to 2^63-1. We can remove the restiction and the note
in some later version.
> > > + 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.
Oh, oops - I see: the comparison with minval can't catch it because it
would convert both args to unsigned.
The present version says:
if ((errno == ERANGE && (val == APR_INT64_MIN || val == APR_INT64_MAX)) ||
val < 0 || (apr_uint64_t)val < minval || (apr_uint64_t)val > maxval)
return svn_error_return(
svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
_("Number '%s' is out of range '[%lu, %lu]'"),
str, minval, maxval));
It forces the comparisons to be unsigned. AFAIK that's the same as C's
rules for signed/unsigned comparisons of the same size anyway.
What's with the "ERANGE && (val == ...)" test? I notice the Posix spec
for 'strtol' says when it sets errno to ERANGE it also returns LONG_MIN
or LONG_MAX, but what's the point of testing that, even if
apr_strtoi64() does the same (which it doesn't document)? When reading
this code, it looks like you're deliberately excluding some other ERANGE
conditions from this test and allowing them to be treated as success
conditions, but I don't think that's what you mean. Strip the redundant
tests!
- Julian
Received on 2010-09-15 18:13:29 CEST