patches to detect GCC diagnostics

[Roland Illig]

Hi Martin,

I'm impressed how much work you have put into the patches for detecting
nonoptimal diagnostics. It takes a long time to read through the
patches, but it's worth it, knowing that it took much longer for you to
prepare the patch, and that I won't have to submit i18n bug reports in
the foreseeable future. :)

----

+      /* Diagnose "arg" (short for "argument" when lazy).  */
+      if (!strncmp (format_chars, "arg", 3)
+	  && (!format_chars[3]
+	      || format_chars[3] == 's'
+	      || ISSPACE (format_chars[3])))

Wouldn't it be sufficient to just check for !ISALNUM(format_chars[3])?
This would also catch "specify args, return type and so on".

I didn't like the magic "n == 3", but after experimenting a bit, I came
to the conclusion that the code you wrote is the best possible.

typo: ponters should be pointers

typo: drective should be directive

Since your code contains the expression strncmp(str, sub, sizeof sub -
1) occurs quite often, I was thinking whether it would be useful to
declare str_startswith, which expresses the actual intent more directly.

> nchars > 1

Better use ngettext in these 7 cases, to account for multiple plural
forms in Arabic, Polish and Russian. :)

> +      /* Diagnose a backtick (grave accent).  */

This diagnostic should explain how to fix this one since it might be
non-obvious.

typo: /* Likewise for gimple.  */ -- should be cgraph_node

typo: be  cdiagnosed -- spurious whitespace? ;)

possible typo: arn't

there is a FIXME after "you can%'t do that"

"ignoring %<asm%>-specifier for non-static local " might be wrong, as
the word "asm-specifier" might come from the C or C++ grammar. Should
this be "%<asm%> specifier", with a space?

Oh no. "%qE is not an %<asm%> qualifier" might destroy my hopes of
merging diagnostics with the same pattern. If some of them need to be
prefixed with "a" and some others with "an", they cannot be merged. Or I
need to make an exception when the "before" string ends in "a" or "an".
Luckily, for "the" and "the" only the pronunciation differs but not the
spelling.

-   "%qE attribute argument %E is not in the range [0, %E)",
-   name, val, align);
+   "%qE attribute argument %E is not in the range %s, %E%c",
+   name, val, "[0", align, ')');

I don't like this one as it is asymmetrical. Either both characters
should be passed as %c, or none of them. I prefer passing none of them
to make the string easier to read for translators.

+   "unsuffixed floating constant");

I'd rather write "unsuffixed floating point constant". (appears multiple
times)

-  warning (OPT_Wpragmas, "#pragma redefine_extname ignored due to "
-      "conflict with __asm__ declaration");
+  warning (OPT_Wpragmas, "%<#pragma redefine_extname%> ignored "
+      "due to conflict with %<asm%> declaration");

Are you sure that you want to remove the underscores? Just asking, I
haven't checked the surrounding code.

-	  error ("#pragma GCC target string... is badly formed");
+	  error ("%<#pragma GCC target%> string is badly formed");
-	  error ("#pragma GCC optimize string... is badly formed");
+	  error ("%<#pragma GCC optimize%> string is badly formed");

I think the "string..." was supposed to go inside the quotes.

+  warning (0, "%s:tag %qx is invalid", filename, tag);

I think there should be a space after the colon, but that should be in
another commit. This one is already big enough.

-    inform (cloc, "%s%#qD <near match>", msg, fn);
+    inform (cloc, "%s%#qD %s", msg, fn, "<near match>");

This change and the similar ones around this will prevent the "<near
match>" string from being translated at all. These strings should stay
in the format string, there needs to be a different solution for them.

-  error_at (loc, "typeid-expression is not a constant expression "
+  error_at (loc, "%<typeid%> is not a constant expression "

This sounds like a term from the C++ grammar.

+  inform (loc, "in C++11 destructors default to %<noexcept%>");

grammar: "defaulting to"? A few lines below there is "because
destructors default", where this word is correct, so it may be a
copy-and-paste mistake.

+  inform (input_location, "  enters %<constexpr if%> statement");

According to https://en.cppreference.com/w/cpp/language/if#Constexpr_If,
the term "constexpr if statement" is a single word.

   error ("explicitly defaulted function %q+D cannot be declared "
 	 "as %<constexpr%> because the implicit declaration is not "
-	 "%<constexpr%>:", fn);
+	 "%qs:", fn, "constexpr");

I don't understand why you extracted one of the %<constexpr%> but left
the other one in the message.

+  "an %<asm%>-specification is not allowed "

See somewhere above.

+  error ("requested %<init_priority%> %i is out of range [0, %i]",
+         pri, MAX_INIT_PRIORITY);

The other places use %u for MAX_INT_PRIORITY since its value is 65535. I
don't know whether GCC would work at all on platforms where %d is 16
bit, as it would be hard to address hundreds of megabytes of heap in
such an environment.

+  (0, "requested %<init_priority%> %i is reserved for internal use",
+  pri);

Same.

+for real_option in -Wstrict-prototypes
-Wmissing-prototypes-Wno-error=format-diag; do

Missing space before -Wno-error.

-      internal_error ("verify_eh_tree failed");
+      internal_error ("%qs failed", __func__);

Nice one. Now that I see it it's the obvious way of reporting this
internal error. Getting the idea is not so obvious. :)

+ error ("conversion of an %qs on the left hand side of %qs",

The article "an" is probably wrong in some cases.

----

Wow. A big impressed Wow that you took the time and effort to change all
these diagnostics. For a patch of this size, I have fewer remarks than I
would have thought.

Roland