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