String formatting with APR_INT64_T etc. & gettext localization

Mattias Engdegård mentioned [1] that nobody has run the 'po-update.sh' script in trunk for some time, as it complains about some format strings it can't handle:

../libsvn_fs_fs/index.c:594: ... string ends in the middle of a directive.
../libsvn_fs_fs/index.c:654: ... string ends in the middle of a directive.
../libsvn_fs_fs/cached_data.c:914: ... string ends in the middle of a directive.

The code in each case is similar to:

  svn_error_createf(... _("... %" APR_UINT64_T_FMT "..."),
                        some_uint64_t_value);

The stupid thing is we have no easy and general solution at the moment.

Ideas:

1. Use PRIu64 etc. from <inttypes.h>.
2. Use a helper function to format an integer as a string; insert the result with "%s".
3. Double-formatting, using "...%%%s..." to insert the correct format specifier.
4. Modify 'gettext' to expand or recognize these macros.
5. Pre-process the source to expand these macros.

1. GNU gettext supports the C'99 PRI... macros from <inttypes.h>:

  #include <inttypes.h>
  svn_error_createf(..., _("... %" PRIu64 "..."),
                         some_uint64_t_value);

Limitations:

  * GNU gettext is hard-coded to recognize  the specific types that are
    defined in <inttypes.h>, so it will not work for other similar
    definitions of our own or from APR such as APR_OFF_T_FMT.

  * We'd have to arrange for <inttypes.h> or equivalent to be
    available on all platforms. (That would be useful anyway, and is not
    fundamentally difficult.)

2. Use a helper function. (We do this in some places.)

  #define ui64toa(pool, i) apr_psprintf(pool, "%"APR_UINT64_T_FMT, i)
  svn_error_createf(..., _("... %s..."),
                         ui64toa(pool, some_uint64_t_value));

Limitations:

  * Requires a pool. Some of the current functions don't have one. We
    could add one, but it's seems a bit stupid to have to do this, when the
    'printf' code already has access to one.

An alternative:

  #define ui64toa(i) apr_psprintf(svn_pool_create(NULL), "%"APR_UINT64_T_FMT, i)
  svn_error_createf(..., _("... %s..."),
                         ui64toa(some_uint64_t_value));

But that doesn't release the allocated memory.

3. Double-formatting. (We do this in some places.)

  svn_error_createf(...,
                    apr_psprintf(pool, _("... %%%s ..."),APR_INT64_T_FMT),
                    some_uint64_t_value);

Limitations:

  * Prevents compile-time checking of argument types, and not just for the
    arguments that need special handling but for all the arguments.

  * Requires a pool.

  * Verbose.

4. Modify gettext.

This could be the best long-term solution. However, I'm not tackling it now.

5. Pre-process the source.

I tried this and it worked.

We can simplify the source code as in the following examples. (The first hunk fixes one of the present problems.)

[[[

Index: subversion/libsvn_fs_fs/cached_data.c
===================================================================
--- subversion/libsvn_fs_fs/cached_data.c    (revision 1589948)
+++ subversion/libsvn_fs_fs/cached_data.c    (working copy)
@@ -913,6 +913,6 @@
         return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
-                                 _("No representation found at offset %s "
-                                   "for item %" APR_UINT64_T_FMT
+                                 _("No representation found at offset %" APR_OFF_T_FMT
+                                   " for item %" APR_UINT64_T_FMT
                                    " in revision %ld"),
-                                 apr_off_t_toa(pool, offset),
+                                 offset,
                                  rep->item_index, rep->revision);
Index: subversion/libsvn_fs_x/noderevs.c
===================================================================
--- subversion/libsvn_fs_x/noderevs.c    (revision 1589948)
+++ subversion/libsvn_fs_x/noderevs.c    (working copy)
@@ -436,6 +436,4 @@
     return svn_error_createf(SVN_ERR_FS_CONTAINER_INDEX, NULL,
-                             apr_psprintf(pool,
-                                          _("Node revision index %%%s"
-                                            " exceeds container size %%d"),
-                                          APR_SIZE_T_FMT),
+                             _("Node revision index %" APR_SIZE_T_FMT
+                               " exceeds container size %d"),
                              idx, container->noderevs->nelts);
]]]

The patch:

[[[
Index: tools/po/po-update.sh
===================================================================
--- tools/po/po-update.sh    (revision 1589948)
+++ tools/po/po-update.sh    (working copy)
@@ -49,2 +49,29 @@

+# Process the content of file "$1" into a new file named "$2".
+preprocess_file()
+{
+  sed < "$1" \
+    -e 's/APR_SSIZE_T_FMT/"ld"/g' \
+    -e 's/APR_SIZE_T_FMT/"lu"/g' \
+    -e 's/APR_OFF_T_FMT/"ld"/g' \
+    -e 's/APR_PID_T_FMT/"d"/g' \
+    -e 's/APR_INT64_T_FMT/PRId64/g' \
+    -e 's/APR_UINT64_T_FMT/PRIu64/g' \
+    -e 's/APR_UINT64_T_HEX_FMT/PRIx64/g' \
+    -e 's/SVN_FILESIZE_T_FMT/PRId64/g' \
+    > "$2"
+}
+
+# For each file name on standard input, process the file's content into a
+# new file and write the new file's name on standard output.
+preprocess_files()
+{
+  while read F; do
+    G="preprocessed/${F#../}"
+    mkdir -p $(dirname "$G")
+    preprocess_file "$F" "$G"
+    echo "$G"
+  done
+}
+
 make_pot()
@@ -54,2 +82,3 @@
     (cd $svn_base/subversion/po && \
+    rm -r preprocessed && \
     find .. \
@@ -61,2 +90,3 @@
     -name "svn_fs_util.h" -print | \
+    preprocess_files | \
     $XGETTEXT --sort-by-file -k_ -kN_ -kQ_:1,2 -kSVN_ERRDEF:3 \
]]]

Thoughts?

- Julian

--
Join WANdisco's free daily demo sessions on Scaling Subversion for the Enterprise
<http://www.wandisco.com/training/webinars>