| Message ID | 3000396a-949c-511a-f666-c0c64a9f99cc@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix -Wformat-diag warnings in rs6000-call.c | expand |
On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: > These warnings, including the suggested fixes are seen on power*-linux builds. > > warning: misspelled term 'builtin function' in format; use 'bult-in function' Are you sure it printed bult-in ? > fatal_error (input_location, > - "internal error: builtin function %qs already processed", > + "internal error: builtin-function %qs already processed", It meant built-in function instead of builtin-function I'm pretty sure. > name); > > rs6000_builtin_decls[(int)code] = t = > @@ -11219,7 +11219,7 @@ altivec_expand_builtin (tree exp, rtx target, bool *expandedp) > { > size_t uns_fcode = (size_t) fcode; > const char *name = rs6000_builtin_info[uns_fcode].name; > - error ("Second argument of %qs must be in the range [0, 3].", name); > + error ("Second argument of %qs must be in the range [0, 3]", name); Diagnostics shouldn't start with capital letter either, so it should be "second ..." > - "internal error: builtin function %qs had an unexpected " > + "internal error: builtin-function %qs had an unexpected " > "return type %qs", name, GET_MODE_NAME (h.mode[0])); > - "internal error: builtin function %qs, argument %d " > + "internal error: builtin-function %qs, argument %d " See above. Jakub
On 1/9/21 11:22 PM, Jakub Jelinek wrote: > On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: >> These warnings, including the suggested fixes are seen on power*-linux builds. >> >> warning: misspelled term 'builtin function' in format; use 'bult-in function' > > Are you sure it printed bult-in ? > >> fatal_error (input_location, >> - "internal error: builtin function %qs already processed", >> + "internal error: builtin-function %qs already processed", > > It meant built-in function instead of builtin-function I'm pretty sure. > >> name); >> >> rs6000_builtin_decls[(int)code] = t = >> @@ -11219,7 +11219,7 @@ altivec_expand_builtin (tree exp, rtx target, bool *expandedp) >> { >> size_t uns_fcode = (size_t) fcode; >> const char *name = rs6000_builtin_info[uns_fcode].name; >> - error ("Second argument of %qs must be in the range [0, 3].", name); >> + error ("Second argument of %qs must be in the range [0, 3]", name); > > Diagnostics shouldn't start with capital letter either, so it should be > "second ..." >> - "internal error: builtin function %qs had an unexpected " >> + "internal error: builtin-function %qs had an unexpected " >> "return type %qs", name, GET_MODE_NAME (h.mode[0])); >> - "internal error: builtin function %qs, argument %d " >> + "internal error: builtin-function %qs, argument %d " > > See above. ahh, my bad. verschlimmbessert ... fixed version attached. While looking at all -Wformat-diag warnings ... ../../src/gcc/emit-rtl.c: In function 'rtx_insn* make_insn_raw(rtx)': ../../src/gcc/emit-rtl.c:4038:25: warning: unquoted identifier or keyword 'emit_insn' in format [-Wformat-diag] 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); | ^~~~~~~~~ ../../src/gcc/emit-rtl.c:4038:46: warning: unquoted identifier or keyword 'emit_jump_insn' in format [-Wformat-diag] 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); | ^~~~~~~~~~~~~~ ../../src/gcc/emit-rtl.c:4038:68: warning: unquoted whitespace character '\x0a' in format [-Wformat-diag] 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); | genautomata has `%s' hardcoded, while other places have %q+F. What is the preferred way? is the newline intended? It's followed by a debug_rtx call. ../../src/gcc/rtl.c:860:42: warning: unquoted sequence of 2 consecutive punctuation characters '',' in format [-Wformat-diag] 860 | ("RTL check: expected elt %d type '%c', have '%c' (rtx %s) in %s, at %s:%d", `%c', or some %q quoting? ../../src/gcc/config/rs6000/rs6000.c: In function 'void rs6000_emit_move(rtx, rtx, machine_mode)': ../../src/gcc/config/rs6000/rs6000.c:10330:16: warning: contraction 'can't' in format; use 'cannot' instead [-Wformat-diag] 10330 | error ("%qs is an opaque type, and you can't set it to other values.", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../src/gcc/config/rs6000/rs6000.c:10330:16: warning: spurious trailing punctuation sequence '.' in format [-Wformat-diag] ../../src/gcc/config/rs6000/rs6000.c: In function 'tree_node* rs6000_handle_altivec_attribute(tree_node**, tree, tree, int, bool*)': ../../src/gcc/config/rs6000/rs6000.c:19811:12: warning: misspelled term 'floating point' in format; use 'floating-point' instead [-Wformat-diag] 19811 | error ("use of decimal floating point types in AltiVec types is invalid"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../src/gcc/config/rs6000/rs6000.c: In function 'tree_node* rs6000_get_function_versions_dispatcher(void*)': ../../src/gcc/config/rs6000/rs6000.c:24597:17: warning: unquoted keyword 'ifunc' in format [-Wformat-diag] 24597 | "multiversioning needs ifunc which is not supported " | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 24598 | "on this target"); again, `' quotes, or some %q option?
On 1/10/21 3:29 AM, Matthias Klose wrote: > On 1/9/21 11:22 PM, Jakub Jelinek wrote: >> On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: >>> These warnings, including the suggested fixes are seen on power*-linux builds. >>> >>> warning: misspelled term 'builtin function' in format; use 'bult-in function' >> >> Are you sure it printed bult-in ? >> >>> fatal_error (input_location, >>> - "internal error: builtin function %qs already processed", >>> + "internal error: builtin-function %qs already processed", >> >> It meant built-in function instead of builtin-function I'm pretty sure. >> >>> name); >>> >>> rs6000_builtin_decls[(int)code] = t = >>> @@ -11219,7 +11219,7 @@ altivec_expand_builtin (tree exp, rtx target, bool *expandedp) >>> { >>> size_t uns_fcode = (size_t) fcode; >>> const char *name = rs6000_builtin_info[uns_fcode].name; >>> - error ("Second argument of %qs must be in the range [0, 3].", name); >>> + error ("Second argument of %qs must be in the range [0, 3]", name); >> >> Diagnostics shouldn't start with capital letter either, so it should be >> "second ..." >>> - "internal error: builtin function %qs had an unexpected " >>> + "internal error: builtin-function %qs had an unexpected " >>> "return type %qs", name, GET_MODE_NAME (h.mode[0])); >>> - "internal error: builtin function %qs, argument %d " >>> + "internal error: builtin-function %qs, argument %d " >> >> See above. > > ahh, my bad. verschlimmbessert ... > > fixed version attached. While looking at all -Wformat-diag warnings ... > > ../../src/gcc/emit-rtl.c: In function 'rtx_insn* make_insn_raw(rtx)': > ../../src/gcc/emit-rtl.c:4038:25: warning: unquoted identifier or keyword > 'emit_insn' in format [-Wformat-diag] > 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); > | ^~~~~~~~~ > ../../src/gcc/emit-rtl.c:4038:46: warning: unquoted identifier or keyword > 'emit_jump_insn' in format [-Wformat-diag] > 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); > | ^~~~~~~~~~~~~~ > ../../src/gcc/emit-rtl.c:4038:68: warning: unquoted whitespace character '\x0a' > in format [-Wformat-diag] > 4038 | warning (0, "ICE: emit_insn used where emit_jump_insn needed:\n"); > | > > genautomata has `%s' hardcoded, while other places have %q+F. What is the > preferred way? > > is the newline intended? It's followed by a debug_rtx call. To avoid the warning there shouldn't be any trailing punctuation or whitespace in the message. The GCC quoting directives should be preferred over the literal characters (as per GCC Coding Conventions). %qc and %qs are preferable to %<%c%>. Symbols/identifiers should be formatted using the appropriate directives or quoted in %< %>. Underscores in words like emit_insn are taken as indicators that the word is an identifier and to trigger warnings. > > ../../src/gcc/rtl.c:860:42: warning: unquoted sequence of 2 consecutive > punctuation characters '',' in format [-Wformat-diag] > 860 | ("RTL check: expected elt %d type '%c', have '%c' (rtx %s) in %s, at > %s:%d", > > `%c', or some %q quoting? The purpose of the -Wformat-diag warnings is to improve the consistency of user-visible messages and make them easier to translate. There was a discussion some time back about whether internal errors should fall into this category. I'm not sure if it reached a conclusion one way or the other but in similar situations elsewhere in GCC we have suppressed the warning via #pragma GCC diagnostic. If it takes too much effort to clean them up it might make sense to do the same here (the downside is that it doesn't help translators). Otherwise, the messages are not really phrased in a way that's comprehensible either to users or to tranlators (acronyms like elt or rtx aren't universally understood). > > ../../src/gcc/config/rs6000/rs6000.c: In function 'void rs6000_emit_move(rtx, > rtx, machine_mode)': > ../../src/gcc/config/rs6000/rs6000.c:10330:16: warning: contraction 'can't' in > format; use 'cannot' instead [-Wformat-diag] > 10330 | error ("%qs is an opaque type, and you can't set it to other > values.", > | > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../src/gcc/config/rs6000/rs6000.c:10330:16: warning: spurious trailing > punctuation sequence '.' in format [-Wformat-diag] > ../../src/gcc/config/rs6000/rs6000.c: In function 'tree_node* > rs6000_handle_altivec_attribute(tree_node**, tree, tree, int, bool*)': > ../../src/gcc/config/rs6000/rs6000.c:19811:12: warning: misspelled term > 'floating point' in format; use 'floating-point' instead [-Wformat-diag] > 19811 | error ("use of decimal floating point types in AltiVec types is > invalid"); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../src/gcc/config/rs6000/rs6000.c: In function 'tree_node* > rs6000_get_function_versions_dispatcher(void*)': > ../../src/gcc/config/rs6000/rs6000.c:24597:17: warning: unquoted keyword 'ifunc' > in format [-Wformat-diag] > 24597 | "multiversioning needs ifunc which is not supported " > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 24598 | "on this target"); > > > again, `' quotes, or some %q option? > The latter: %qs with an argument is best in general (it can reduce translation effort between repeated messages parameterized on the quoted string). Martin
On 1/10/21 10:18 PM, Martin Sebor wrote: > On 1/10/21 3:29 AM, Matthias Klose wrote: >> is the newline intended? It's followed by a debug_rtx call. > > To avoid the warning there shouldn't be any trailing punctuation > or whitespace in the message. The GCC quoting directives should > be preferred over the literal characters (as per GCC Coding > Conventions). %qc and %qs are preferable to %<%c%>. > > Symbols/identifiers should be formatted using the appropriate > directives or quoted in %< %>. Underscores in words like > emit_insn are taken as indicators that the word is an identifier > and to trigger warnings. is this? https://gcc.gnu.org/codingconventions.html#Diagnostics I think that's a bit terse, and grepping sources for %< shows many more occurences than %qX. >> ../../src/gcc/rtl.c:860:42: warning: unquoted sequence of 2 consecutive >> punctuation characters '',' in format [-Wformat-diag] >> 860 | ("RTL check: expected elt %d type '%c', have '%c' (rtx %s) in %s, at >> %s:%d", >> >> `%c', or some %q quoting? > > The purpose of the -Wformat-diag warnings is to improve the consistency > of user-visible messages and make them easier to translate. There was > a discussion some time back about whether internal errors should fall > into this category. I'm not sure if it reached a conclusion one way > or the other but in similar situations elsewhere in GCC we have > suppressed the warning via #pragma GCC diagnostic. If it takes too > much effort to clean them up it might make sense to do the same here > (the downside is that it doesn't help translators). Otherwise, > the messages are not really phrased in a way that's comprehensible > either to users or to tranlators (acronyms like elt or rtx aren't universally > understood). [...] >> again, `' quotes, or some %q option? >> > > The latter: %qs with an argument is best in general (it can reduce > translation effort between repeated messages parameterized on > the quoted string). if the URL above is the correct place for the conventions, then maybe make it more explicit there about the preferred choice. Matthias
On 1/11/21 9:30 AM, Matthias Klose wrote: > On 1/10/21 10:18 PM, Martin Sebor wrote: >> On 1/10/21 3:29 AM, Matthias Klose wrote: >>> is the newline intended? It's followed by a debug_rtx call. >> >> To avoid the warning there shouldn't be any trailing punctuation >> or whitespace in the message. The GCC quoting directives should >> be preferred over the literal characters (as per GCC Coding >> Conventions). %qc and %qs are preferable to %<%c%>. >> >> Symbols/identifiers should be formatted using the appropriate >> directives or quoted in %< %>. Underscores in words like >> emit_insn are taken as indicators that the word is an identifier >> and to trigger warnings. > > is this? > https://gcc.gnu.org/codingconventions.html#Diagnostics > > I think that's a bit terse, and grepping sources for %< shows many more > occurences than %qX. Agreed, the section could stand to be expanded. I don't know about the metrics. There are lots of %<xxx%> but only a few %<%[cs]%>: $ grep " %<%[cs]%> " /src/gcc/master/gcc/po/gcc.pot "cannot apply %<%s%> to %qD, which has also been marked with an OpenMP " "incompatible %qs clause when applying %<%s%> to %qD, which has already been " "missing %qs clause when applying %<%s%> to %qD, which has already been " msgid "unbalanced punctuation character %<%c%> in format" msgid "unterminated quote character %<%c%> in format" msgid "bad value %<%s%> for %<-mtls-size=%> switch" msgid "unexpected %<%s%> after %<%s%>" msgid "invalid argument %<%s%> for %<-mharden-sls=%>" msgid "invalid argument %<%s%> for %<-mbranch-protection=%>" msgid "Unexpected %<%c%> for nonderived-type variable %qs at %C" These should still be changed to %qc and %qs at some point. > >>> ../../src/gcc/rtl.c:860:42: warning: unquoted sequence of 2 consecutive >>> punctuation characters '',' in format [-Wformat-diag] >>> 860 | ("RTL check: expected elt %d type '%c', have '%c' (rtx %s) in %s, at >>> %s:%d", >>> >>> `%c', or some %q quoting? >> >> The purpose of the -Wformat-diag warnings is to improve the consistency >> of user-visible messages and make them easier to translate. There was >> a discussion some time back about whether internal errors should fall >> into this category. I'm not sure if it reached a conclusion one way >> or the other but in similar situations elsewhere in GCC we have >> suppressed the warning via #pragma GCC diagnostic. If it takes too >> much effort to clean them up it might make sense to do the same here >> (the downside is that it doesn't help translators). Otherwise, >> the messages are not really phrased in a way that's comprehensible >> either to users or to tranlators (acronyms like elt or rtx aren't universally >> understood). > > [...] > >>> again, `' quotes, or some %q option? >>> >> >> The latter: %qs with an argument is best in general (it can reduce >> translation effort between repeated messages parameterized on >> the quoted string). > > if the URL above is the correct place for the conventions, then maybe make it > more explicit there about the preferred choice. Yes, I can make that change. My hope is that the warning would make the preferred choice clear in the followup note so if/where it doesn't please let me know (or open a bug) and I'll adjust it. (Otherwise, if there's no warning, there's no preference :) Martin
On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: > These warnings, including the suggested fixes are seen on power*-linux builds. > > warning: misspelled term 'builtin function' in format; use 'bult-in function' > instead [-Wformat-diag] This one is wrong. Almost all the functions are called __builtin_xxx so changing the warning message like that is just silly. Instead, just rephrase it so you do not need to repeat it, The warning message itself is buggy: it pretends it knows what it is talking about. No warning ever does, that is the difference between warnings and errors. The hint (after the semi-colon) is extra wrong. > warning: spurious trailing punctuation sequence '].' in format [-Wformat-diag] This is also wrong (it says error ("Second argument of %qs must be in the range [0, 3].", name); which is obviously correct: "]" is not punctuation, and that makes the error message (again, not phrased as a warning) incorrect as well. > warning: misspelled term 'floating point' in format; use 'floating-point' > instead [-Wformat-diag] It is a a noun here, which is spelled without dash. The warning is mistaken, and its advice (not phrased as advice) is bad advice. [-- Attachment #2: format-diag.diff --] [-- Type: text/x-patch, Encoding: quoted-printable, Size: 2.2K --] Please use plain text attachments, so that people can a) read it on the mail archives, and b) apply the patches easily, etc. Segher
Hi! On Sun, Jan 10, 2021 at 02:18:24PM -0700, Martin Sebor wrote: > Symbols/identifiers should be formatted using the appropriate > directives or quoted in %< %>. We do not have a way to mark up the mathematical symbols [], but we do require those to express ranges (which is a bad idea /an sich/, but that aside). > The purpose of the -Wformat-diag warnings is to improve the consistency > of user-visible messages Which is not a good idea in and of itself. It *is* good to identify bad practices, of course, but to make the GCC feedback more dull and formalised only makes it harder to find where an error came from (and seeing that as a positive thing requires a special kind of mind already ;-) ). > and make them easier to translate. That can be helpful. > There was > a discussion some time back about whether internal errors should fall > into this category. IMNSHO it is counter-productive to translate internal messages at all. Can we automatically mark those as non-translated? > I'm not sure if it reached a conclusion one way > or the other but in similar situations elsewhere in GCC we have > suppressed the warning via #pragma GCC diagnostic. This isn't needed then either. Win-win-win! > If it takes too > much effort to clean them up it might make sense to do the same here > (the downside is that it doesn't help translators). Otherwise, > the messages are not really phrased in a way that's comprehensible > either to users or to tranlators (acronyms like elt or rtx aren't > universally understood). Neither translators or even users are an audience for those messages at all: the only thing they should see is "internal error", and perhaps how to report it. Reporting a translated version of the message is a bad idea. Segher
On Tue, 12 Jan 2021, Segher Boessenkool wrote: > On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: > > These warnings, including the suggested fixes are seen on power*-linux builds. > > > > warning: misspelled term 'builtin function' in format; use 'bult-in function' > > instead [-Wformat-diag] > > This one is wrong. Almost all the functions are called __builtin_xxx > so changing the warning message like that is just silly. Instead, just > rephrase it so you do not need to repeat it, As documented in codingconventions.html we use "built-in" as an adjective in English text (as opposed to literal sequences of characters from C source code). Text in diagnostics that is meant to be source code rather than English words should be enclosed in %<%>. A literal %<__builtin%> should be unchanged in translations, whereas "built-in" used as an adjective should be translated. > > warning: spurious trailing punctuation sequence '].' in format [-Wformat-diag] > > This is also wrong (it says > error ("Second argument of %qs must be in the range [0, 3].", name); > which is obviously correct: "]" is not punctuation, and that makes the > error message (again, not phrased as a warning) incorrect as well. The leading capital letter is incorrect, as is the trailing '.'; both of those are contrary to the GNU Coding Standards for diagnostics. Once those are fixed, having a trailing ']' is fine.
Hi! On Wed, Jan 13, 2021 at 12:49:42AM +0000, Joseph Myers wrote: > On Tue, 12 Jan 2021, Segher Boessenkool wrote: > > On Sat, Jan 09, 2021 at 07:44:31PM +0100, Matthias Klose wrote: > > > These warnings, including the suggested fixes are seen on power*-linux builds. > > > > > > warning: misspelled term 'builtin function' in format; use 'bult-in function' > > > instead [-Wformat-diag] > > > > This one is wrong. Almost all the functions are called __builtin_xxx > > so changing the warning message like that is just silly. Instead, just > > rephrase it so you do not need to repeat it, > > As documented in codingconventions.html we use "built-in" as an adjective > in English text (as opposed to literal sequences of characters from C > source code). Text in diagnostics that is meant to be source code rather > than English words should be enclosed in %<%>. A literal %<__builtin%> > should be unchanged in translations, whereas "built-in" used as an > adjective should be translated. My point is that "built-in function `__builtin_blablabla'" looks silly. It is much better to not mention "built-in" again (it isn't useful anyway). > > > warning: spurious trailing punctuation sequence '].' in format [-Wformat-diag] > > > > This is also wrong (it says > > error ("Second argument of %qs must be in the range [0, 3].", name); > > which is obviously correct: "]" is not punctuation, and that makes the > > error message (again, not phrased as a warning) incorrect as well. > > The leading capital letter is incorrect, as is the trailing '.'; both of > those are contrary to the GNU Coding Standards for diagnostics. Yes, and yes. But the warning message is *wrong*, that is my point. It says "]." is a punctuation sequence, and it is not (the "[" isn't punctuation at all, it is a mathematical symbol). I am not saying the diagnostics could not be improved, they certainly could. But the warning messages are just wrong on many levels as well, and the suggested "fixes" make things worse in many cases (and the messages do not say "maybe" or "we suggest" or similar). Thanks, Segher
diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 2308cc8b4a2..3988dd81481 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -8741,7 +8741,7 @@ def_builtin (const char *name, tree type, enum rs6000_builtins code) if (rs6000_builtin_decls[(int)code]) fatal_error (input_location, - "internal error: builtin function %qs already processed", + "internal error: builtin-function %qs already processed", name); rs6000_builtin_decls[(int)code] = t = @@ -11219,7 +11219,7 @@ altivec_expand_builtin (tree exp, rtx target, bool *expandedp) { size_t uns_fcode = (size_t) fcode; const char *name = rs6000_builtin_info[uns_fcode].name; - error ("Second argument of %qs must be in the range [0, 3].", name); + error ("Second argument of %qs must be in the range [0, 3]", name); return expand_call (exp, target, false); } break; @@ -11479,7 +11479,7 @@ rs6000_invalid_builtin (enum rs6000_builtins fncode) else if ((fnmask & RS6000_BTM_HARD_FLOAT) != 0) error ("%qs requires the %qs option", name, "-mhard-float"); else if ((fnmask & RS6000_BTM_FLOAT128_HW) != 0) - error ("%qs requires ISA 3.0 IEEE 128-bit floating point", name); + error ("%qs requires ISA 3.0 IEEE 128-bit floating-point", name); else if ((fnmask & RS6000_BTM_FLOAT128) != 0) error ("%qs requires the %qs option", name, "%<-mfloat128%>"); else if ((fnmask & (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64)) @@ -14582,7 +14582,7 @@ builtin_function_type (machine_mode mode_ret, machine_mode mode_arg0, if (!ret_type) fatal_error (input_location, - "internal error: builtin function %qs had an unexpected " + "internal error: builtin-function %qs had an unexpected " "return type %qs", name, GET_MODE_NAME (h.mode[0])); for (i = 0; i < (int) ARRAY_SIZE (arg_type); i++) @@ -14604,7 +14604,7 @@ builtin_function_type (machine_mode mode_ret, machine_mode mode_arg0, if (!arg_type[i]) fatal_error (input_location, - "internal error: builtin function %qs, argument %d " + "internal error: builtin-function %qs, argument %d " "had unexpected argument type %qs", name, i, GET_MODE_NAME (m)); }