On Wed, Sep 15, 2010 at 05:12:43PM +0100, Julian Foad wrote:
> > > > + * @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.)
Should we use INT_MIN and INT_MAX so?
> > > > 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.
If we allowed ourselves to use strtoull() directly instead of using
apr_strtoi64(), this problem would go away. Apparently it's part of C99
only though. If we decide to do this, I'll skip apr_strtoi64() as well,
and just call strtoll() directly.
We could also use strtol() and strtoul(), which are C89.
> > > > + 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.
It might be, but I can't keep all rules of C in my head.
So it's easier to be explicit.
> 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!
POSIX does that to allow the caller to tell whether underflow or
overflow occured. It doesn't really matter since we treat both cases
the same way. I kept this for consistency with programming examples
I found for strtol(), e.g. in OpenBSD's strtol manual page:
http://www.openbsd.org/cgi-bin/man.cgi?query=strtol&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
Stefan
Received on 2010-09-15 21:01:20 CEST