On 03/01/2018 04:25 PM, Jakub Jelinek wrote:
Hi!
This patch fixes a couple of i18n bugs in gimple-ssa-sprintf.c.
One issue was incorrect format attribute for format_warning_at_substring,
which has ... and thus shouldn't use 0 for the last argument, but the first
non-named argument position.
Another one is that a couple of fmtwarn calls need plural form handling,
so I've introduced new substring-locations.[ch] entrypoints for that;
in order to be able to successfully extract the plural form translatable
strings from those, it can't use the const function pointer hack to shorten
the function name, nor can use the conditionals to pick the format strings.
Another thing is that the file had
#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
which has hidden a couple of issues, missing ATTRIBUTE_GCC_DIAG on the
const fn pointer and lots of missing casts to int for size_t arguments
passed as length to %.*s.
Also, if we use the const fn pointer hack to shorten the function name,
exgettext doesn't really work on it, so I had to add G_(...) around a couple
of non-conditional format strings, the conditional ones already had it.
Though for those I got rid of the fmtstr temporaries which just caused
gcc not to see the format strings and be able to diagnose issues; plus is
bad for -Wformat-security too.
I've often wished -Wformat were smarter and be able to get at
the format strings in expressions like this. With only a little
bit of help from the programmer it should be possible. Just
declaring the pointer const should be enough for it to get them
from its initializer. This would benefit not just -Wformat but
also functions like strlen that could be folded early on, well
before the strlen pass. I'll look into it in stage 1.
Bootstrapped/regtested on x86_64-linux and i686-linux, additionally tested
with
make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}
tree-ssa.exp=builtin-sprintf*'
with unpatched and patched compiler and diffed gcc/testsuite/gcc/gcc.log in
between the two to make sure no diagnostics differences. And also run make
gcc.pot and verified all the strings I expected to be translatable to be
there, in the desired form (plural-form vs. normal). Ok for trunk?
Thanks for working on this. I would only request to do something
about the exceedingly long names of the new functions. They make
some of the new warning messages even harder to read than before
by breaking up the strings across even more lines. IIRC, that's
what I was trying to avoid with the pointer hack. How about
adding a fmtwarn_n pointer like fmtwarn? (There's little point
in having one without the other.)
--- gcc/gimple-ssa-sprintf.c.jj 2018-02-27 23:16:19.747948912 +0100
+++ gcc/gimple-ssa-sprintf.c 2018-03-01 21:26:37.728861287 +0100
@@ -592,14 +592,12 @@ get_format_string (tree format, location
/* The format_warning_at_substring function is not used here in a way
that makes using attribute format viable. Suppress the warning. */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
-
The comment above the #pragmas should be removed too.
/* For convenience and brevity. */
static bool
(* const fmtwarn) (const substring_loc &, location_t,
const char *, int, const char *, ...)
+ ATTRIBUTE_GCC_DIAG (5, 6)
= format_warning_at_substring;
So add
static bool
(* const fmtwarn_n) (const substring_loc &, location_t,
const char *, int, unsigned HOST_WIDE_INT,
const char *, const char*, ...)
ATTRIBUTE_GCC_DIAG (5, 6)
= format_warning_at_substring_n;
and use the pointer instead?
Martin
