On Mon, Jun 2, 2014 at 2:40 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>
> 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,
> > > 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.
> > >> /* copy the info */
> > >> - return l2p_page_info_copy(baton, header, page_table,
> > >> + 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
> > > 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
> > - " revision %ld"),
> > + _("Item index %s too large "
> > + "in l2p proto index for revision
> 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
> " 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!
Received on 2014-06-02 11:38:50 CEST