On Wed, 2010-09-15 at 20:59 +0200, Stefan Sperling wrote:
> 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?
Yes, if they are standard (I mean if they are always available).
> > > > > 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
OK, judging by reading the web, this seems to be a case where people are
unsure whether the C standard allows the implementation to set errno to
ERANGE when there's not an overflow.
- Julian
Received on 2010-09-16 15:52:16 CEST