On 5/16/19 8:58 AM, Roland Illig wrote:
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. :)
Thanks. That's the idea :)
----
+ /* 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've improved the test in the revised patch.
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
Fixed, thanks.
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.
Good idea, thanks!
nchars > 1
Better use ngettext in these 7 cases, to account for multiple plural
forms in Arabic, Polish and Russian. :)
Ah, right. That's exactly why a checker for this would be so
handy here! :) I've reworded the warning to avoid the plural
vs singular difference.
+ /* Diagnose a backtick (grave accent). */
This diagnostic should explain how to fix this one since it might be
non-obvious.
Done (I assume we prefer an apostrophe instead).
typo: /* Likewise for gimple. */ -- should be cgraph_node
typo: be cdiagnosed -- spurious whitespace? ;)
possible typo: arn't
Thanks. As might be apparent from all these typos, I probably
need a checker like this more than anyone.
there is a FIXME after "you can%'t do that"
I've added the detection.
"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?
Yes, I think the dash probably shouldn't be there. It isn't
in references to "%<asm%> qualifiers." Let me remove it.
Oh no. "%qE is not an %<asm%> qualifier" might destroy my hopes of
merging diagnostics with the same pattern.
How about rephrasing as:
"%qE is not a valid qualifier for the %<asm%> keyword"
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.
I don't like this one either and was tempted to change it to
the more common [min, max] form. I can do that, it just means
extracting the integer from align:
"%qE attribute argument %E is not in the range [0, %wu]",
name, val, tree_to_uhwi (align));
> + "unsuffixed floating constant");
I'd rather write "unsuffixed floating point constant". (appears multiple
times)
This came from the C standard that refers to "floating constants."
To be fair, C also refers to both "floating types" and "floating
point types" but not to "floating point constant." I'm inclined
to leave it as is just because it's shorter and means less work,
but I'm not dead set against changing it either if it's important.
- 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.
They should be interchangeable.
- 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.
The "string" refers to the pragma argument and quoting it would
imply that the word string should be verbatim. So I think it's
correct as is, despite the call above:
GCC_BAD ("%<#pragma GCC optimize (string [,string]...)%> does "
"not have a final %<)%>");
where the string is part of the grammar being shown.
+ 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.
Yes, let's revisit this later. I'd prefer to change only what
the checker complains about in the first commit. There are
other warnings like this below that should be adjusted if we
decide the change above is appropriate.
- 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.
Ugh. I forgot about that. Let me see how to fix that. I'd
prefer not to have treat < and > as (unquoted) brackets in plain
text.
- 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.
It does but there is no /typeid-expression/ in the C++ grammar.
It's just %<typeid%> expression. I took out the word "expression"
because it seemed redundant with the "constant expression" that
follows.
That said, it could be handy to be able to uniquely distinguish
grammar terms (do you translate C/C++ grammar terms or leave them
in English?). I'd like to add either a new directive or flag to
%< to let us do that. E.g. with something like:
"%#<postfx-expression%#>"
GCC could render the term in italics (there's a bug where we
discussed this that I can't find right now).
+ 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.
I think "default to" is correct. What might be worth changing
for the sake of consistency is the "because": either add it
above or remove it below.
+ 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.
The quoting in the message above is corresponds to the C++ standard
so I'll leave it.
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.
Fixed.
+ "an %<asm%>-specification is not allowed "
See somewhere above.
I've removed the dash.
+ 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.
pri's type is int so %i should be fine.
+for real_option in -Wstrict-prototypes
-Wmissing-prototypes-Wno-error=format-diag; do
Missing space before -Wno-error.
Yikes! Good catch!
- 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.
It's always "SSA_NAME" but I've removed the article. It doesn't
need to be there.
----
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.
Thanks, and thank you also for the careful review!
Let me post an updated patch with the warning changes. I will
also make the adjustments you suggested above to the diagnostics
in the other files and commit them after retesting everything
if/when they are approved (I see Jeff has already acked a few)
unless someone wants to see an updated patch.
Martin
