On Mon, Jun 2, 2014 at 2:40 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> 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.
>
Committed as r1599140, with a few formatting and docstring fixes.
Thanks for the patch & review!
-- Stefan^2.
Received on 2014-06-02 11:38:50 CEST