Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: local policy)
Date: Sun, 1 Jun 2014 01:53:24 +0000
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Andreas Stieger <andreas.stieger@gmx.de>
Cc: dev@subversion.apache.org
Subject: Re: [PATCH] Fix xgettext warnings and incomplete format strings due
 to macros
Message-ID: <20140601015324.GA2052@tarsus.local2>
References: <538A50FB.9030500@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <538A50FB.9030500@gmx.de>
User-Agent: Mutt/1.5.21 (2010-09-15)

> Index: subversion/libsvn_fs_fs/cached_data.c
> ===================================================================
> --- subversion/libsvn_fs_fs/cached_data.c	(revision 1598897)
> +++ subversion/libsvn_fs_fs/cached_data.c	(working copy)
> @@ -941,9 +941,10 @@ svn_fs_fs__check_rep(representation_t *rep,
>            || entry->type < SVN_FS_FS__ITEM_TYPE_FILE_REP
>            || entry->type > SVN_FS_FS__ITEM_TYPE_DIR_PROPS)
>          return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
> -                                 _("No representation found at offset %s "
> -                                   "for item %" APR_UINT64_T_FMT
> -                                   " in revision %ld"),
> +                                 apr_psprintf(pool,
> +                                   _("No representation found at offset %%s "
> +                                     "for item %%%s in revision %%ld"),
> +                                 APR_UINT64_T_FMT),
>                                   apr_off_t_toa(pool, offset),
>                                   rep->item_index, rep->revision);
>  

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?

> Index: subversion/libsvn_fs_fs/index.c
> ===================================================================
> --- subversion/libsvn_fs_fs/index.c	(revision 1598897)
> +++ subversion/libsvn_fs_fs/index.c	(working copy)
>  l2p_page_info_copy(l2p_page_info_baton_t *baton,
>                     const l2p_header_t *header,
>                     const l2p_page_table_entry_t *page_table,
> -                   const apr_size_t *page_table_index)
> +                   const apr_size_t *page_table_index,
> +                   apr_pool_t *pool)
>  {
>    /* revision offset within the index file */
>    apr_size_t rel_revision = baton->revision - header->first_revision;
> @@ -932,9 +936,10 @@ l2p_page_info_copy(l2p_page_info_baton_t *baton,
>                         * (last_entry - first_entry);
>        if (baton->item_index >= max_item_index)
>          return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
> -                                _("Item index %" APR_UINT64_T_FMT
> -                                  " exceeds l2p limit of %" APR_UINT64_T_FMT
> -                                  " for revision %ld"),
> +                                apr_psprintf(pool,
> +                                  _("Item index %%%s exceeds l2p limit "
> +                                    "of %%%s for revision %%ld"),
> +                                APR_UINT64_T_FMT, APR_UINT64_T_FMT),
>                                  baton->item_index, max_item_index,
>                                  baton->revision);
>  
> @@ -970,7 +975,7 @@ l2p_page_info_access_func(void **out,
>                             (const void *const *)&header->page_table_index);
>  
>    /* 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.)

Thanks,

Daniel