Andreas Stieger wrote on Sun, Jun 01, 2014 at 21:22:15 +0100:
> On 01/06/14 02:53, Daniel Shahaf wrote:
> > Don't we prefer doing:
> >
> > svn_error_createf(SVN_ERR_BASE, NULL, _("%s: number %ld"),
> > apr_psprintf(pool, "%" APR_UINT64_T_FMT,
> > (apr_uint64_t)1),
> > 1L)
> >
> > since it allows for compile-time type checking of varargs against the
> > format string?
>
> Yes, adjusted accordingly, and fixed another instance of same in
> l2p_page_get_offset which was previously attempted by someone else.
>
Thanks.
> >> /* copy the info */
> >> - return l2p_page_info_copy(baton, header, page_table, page_table_index);
> >> + return l2p_page_info_copy(baton, header, page_table, page_table_index,
> >> result_pool);
> >
> > That should be scratch_pool, since you only use it to allocate an error
> > message. (The svn_error_t->message member is constructed in the error's
> > pool, which is the child of a global pool, not related to any of the
> > pools in this scope.)
>
> As discussed on IRC, a scratch_pool in not available. Changed to
> local_pool where it is created in the function. Again review for the
> updated patch would be appreciated, especially on the pool usage in the
> context.
>
Yes, sorry for not spotting that that function doesn't have a
scratch_pool. I assumed one would be available, so I didn't check :-(
Reviewed (including pool usage in context), +1 to commit.
Two minor nitpicks:
> [[[
> * subversion/libsvn_fs_fs/cached_data.c
> (svn_fs_fs__check_rep): make xgettext friendly by removing the
> macro from the format string
>
It would be clearer if "xgettext-friendly" were hyphenated.
> - _("Item index %" APR_UINT64_T_FMT
> - " too large in l2p proto index for"
> - " revision %ld"),
> + _("Item index %s too large "
> + "in l2p proto index for revision %ld"),
We generally try to minimize whitespace changes in patches that make
functional changes; it makes for cleaner diffs and cleaner 'svn blame'
output. In this instance, not rewrapping the message ---
- _("Item index %" APR_UINT64_T_FMT
+ _("Item index %s
" too large in l2p proto index for"
" revision %ld"),
- ...
+ ...
--- might have been preferable; but it's not a major issue.
As I said, +1 to commit.
Thanks for the patch,
Daniel
