You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/09/22 10:38:58 UTC
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
stylesen@apache.org wrote on Wed, Sep 22, 2010 at 07:17:51 -0000:
> Author: stylesen
> Date: Wed Sep 22 07:17:51 2010
> New Revision: 999785
>
> URL: http://svn.apache.org/viewvc?rev=999785&view=rev
> Log:
> Fix a warning.
>
> * subversion/libsvn_subr/svn_string.c
> (svn_cstring_strtoui64, svn_cstring_strtoi64): Use proper format
> specifiers while returning the error.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/svn_string.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/svn_string.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/svn_string.c?rev=999785&r1=999784&r2=999785&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/svn_string.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/svn_string.c Wed Sep 22 07:17:51 2010
> @@ -661,7 +661,7 @@ svn_cstring_strtoui64(apr_uint64_t *n, c
> 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]'"),
> + _("Number '%s' is out of range '[%llu, %llu]'"),
> str, minval, maxval));
minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
> *n = val;
> return SVN_NO_ERROR;
> @@ -705,7 +705,7 @@ svn_cstring_strtoi64(apr_int64_t *n, con
> val < minval || val > maxval)
> return svn_error_return(
> svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> - _("Number '%s' is out of range '[%ld, %ld]'"),
> + _("Number '%s' is out of range '[%lld, %lld]'"),
> str, minval, maxval));
> *n = val;
> return SVN_NO_ERROR;
>
>
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
> > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
> >
> > See this thread
> >
> > http://svn.haxx.se/dev/archive-2010-09/0295.shtml
>
> and subversion/libsvn_fs_fs/rep-cache.c:153
Which is one of the ways it can be done when a pool is available:
> return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> apr_psprintf(pool,
> _("Representation key for checksum '%%s' exists "
> "in filesystem '%%s' with a different value "
> "(%%ld,%%%s,%%%s,%%%s) than what we were about "
> "to store (%%ld,%%%s,%%%s,%%%s)"),
> APR_OFF_T_FMT, SVN_FILESIZE_T_FMT,
> SVN_FILESIZE_T_FMT, APR_OFF_T_FMT,
> SVN_FILESIZE_T_FMT, SVN_FILESIZE_T_FMT),
> svn_checksum_to_cstring_display(rep->sha1_checksum, pool),
> fs->path, old_rep->revision, old_rep->offset, old_rep->size,
> old_rep->expanded_size, rep->revision, rep->offset, rep->size,
> rep->expanded_size);
That's one way to do it. I'd prefer to format the various fields into
one or more non-localized strings and then include those strings in the
final localized string. For example:
/* Return a string containing the interesting fields of REP as a
comma-separated tuple, allocated in POOL. */
static char *
rep_tuple_psprintf(const rep_t *rep, apr_pool_t *pool)
{
return apr_psprintf(pool,
"%ld,%" APR_OFF_T_FMT ",%" SVN_FILESIZE_T_FMT
",%" SVN_FILESIZE_T_FMT,
rep->revision, rep->offset, rep->size,
rep->expanded_size);
}
followed by
return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
apr_psprintf(pool,
_("Representation key for checksum '%%s' exists "
"in filesystem '%%s' with a different value "
"(%s) than what we were about to store (%s)"),
svn_checksum_to_cstring_display(rep->sha1_checksum, pool),
fs->path, old_rep, rep);
- Julian
Re: svn commit: r999785 - /subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> It might be worth adding a pool parameter so that any of the
> print-into-a-substing variants can be used.
Perhaps we can avoid these complications in the code. Could we invoke
the C pre-processor during the po-update stuff? Perhaps on the output
of gettext?
--
Philip
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
> >> Daniel Shahaf <d....@daniel.shahaf.name> writes:
> >> > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
> >>
> >> See this thread
> >>
> >> http://svn.haxx.se/dev/archive-2010-09/0295.shtml
> >>
> >
> > and subversion/libsvn_fs_fs/rep-cache.c:153
>
> Yes, that would be another way to do it. I think it will defeat the
> compiler's compile-time printf parameter checking, only the inner
> printf would get checked.
It might be worth adding a pool parameter so that any of the
print-into-a-substing variants can be used.
- Julian
Re: svn commit: r999785 - /subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
>> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
>>
>> See this thread
>>
>> http://svn.haxx.se/dev/archive-2010-09/0295.shtml
>>
>
> and subversion/libsvn_fs_fs/rep-cache.c:153
Yes, that would be another way to do it. I think it will defeat the
compiler's compile-time printf parameter checking, only the inner
printf would get checked.
--
Philip
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Sep 22, 2010 at 14:15:52 +0200:
> On Wed, Sep 22, 2010 at 12:59:49PM +0200, Daniel Shahaf wrote:
> > Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
> > > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > > > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
> > >
> > > See this thread
> > >
> > > http://svn.haxx.se/dev/archive-2010-09/0295.shtml
> > >
> >
> > and subversion/libsvn_fs_fs/rep-cache.c:153
>
> Daniel, the number parsing functions don't have a pool needed for the
> apr_psprintf() call, so they cannot use this approach.
>
Ah, right.
Although... in fact the pool is only needed for a constant-size
allocation (the format codes are three characters or less), not for an
arbitrary-sized one, so presumably we could arrange it to work even
without a pool (by allocating a format string with placeholders of the
form "foo%...bar%..." off the stack; the error is always allocated in
a global pool).
Not saying it's a good idea, just that it could theoreticaly work :)
> I'd say let's just stick to %ld for now, i.e. revert r999785.
>
But isn't that not always the correct format code?
> What we really need to do here is to decide whether or not we want to
> use APR for this at all. See this and related posts:
> http://svn.haxx.se/dev/archive-2010-09/0306.shtml
> The APR interface causes format string problems and lacks support for
> unsigned types. The C99 functions don't have these problems.
>
> As Philip pointed out, the format string problem is fixable by
> checking sizeof(long) at runtime. And we could request support for unsigned
> types in the APR interface, or even send a patch there. But that still
> leaves us with the question of what to do with current and older APR versions.
>
> I suppose the least painful route is to use C89/C99 constructs and switch
> all our svn_cstring number parsing functions over to using C89/C99 APIs
> instead of using APR.
> If we find that using these particular C99 functions (strtoll and stroull)
> or types (long long, which has been present in some compilers before C99),
> breaks Subversion with any compiler or C runtime in use today, we can add
> our own implementation for compatibility (BSD-licenced code is available),
> or fall back to APR on those platforms.
>
Sounds good. (to use the C99 functions, with their better-than-APR's
support for unsigned types, and add a private implementation if it's
needed)
> Stefan
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 22, 2010 at 12:59:49PM +0200, Daniel Shahaf wrote:
> Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
> > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
> >
> > See this thread
> >
> > http://svn.haxx.se/dev/archive-2010-09/0295.shtml
> >
>
> and subversion/libsvn_fs_fs/rep-cache.c:153
Daniel, the number parsing functions don't have a pool needed for the
apr_psprintf() call, so they cannot use this approach.
I'd say let's just stick to %ld for now, i.e. revert r999785.
What we really need to do here is to decide whether or not we want to
use APR for this at all. See this and related posts:
http://svn.haxx.se/dev/archive-2010-09/0306.shtml
The APR interface causes format string problems and lacks support for
unsigned types. The C99 functions don't have these problems.
As Philip pointed out, the format string problem is fixable by
checking sizeof(long) at runtime. And we could request support for unsigned
types in the APR interface, or even send a patch there. But that still
leaves us with the question of what to do with current and older APR versions.
I suppose the least painful route is to use C89/C99 constructs and switch
all our svn_cstring number parsing functions over to using C89/C99 APIs
instead of using APR.
If we find that using these particular C99 functions (strtoll and stroull)
or types (long long, which has been present in some compilers before C99),
breaks Subversion with any compiler or C runtime in use today, we can add
our own implementation for compatibility (BSD-licenced code is available),
or fall back to APR on those platforms.
Stefan
Re: svn commit: r999785 -
/subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, Sep 22, 2010 at 11:47:19 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
>
> See this thread
>
> http://svn.haxx.se/dev/archive-2010-09/0295.shtml
>
and subversion/libsvn_fs_fs/rep-cache.c:153
Re: svn commit: r999785 - /subversion/trunk/subversion/libsvn_subr/svn_string.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> return svn_error_return(
>> svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
>> - _("Number '%s' is out of range '[%lu, %lu]'"),
>> + _("Number '%s' is out of range '[%llu, %llu]'"),
llu is not better than lu.
>> str, minval, maxval));
>
> minval is an apr_uint64_t, so shouldn't the format string use APR_UINT64_FMT?
See this thread
http://svn.haxx.se/dev/archive-2010-09/0295.shtml
--
Philip