diff mbox series

Remove some further trailing whitespaces from diagnostic messages (PR translation/90011)

Message ID 20190409072119.GK21066@tucnak
State New
Headers show
Series Remove some further trailing whitespaces from diagnostic messages (PR translation/90011) | expand

Commit Message

Jakub Jelinek April 9, 2019, 7:21 a.m. UTC 
Hi!

Several further spots with trailing whitespace, only bootstrapped/regtested
on x86_64-linux and i686-linux (so the ipa-devirt.c change is covered),
the rest is just by eyeballing gcc.pot.

Ok for trunk?

I wonder where and how we could check for this kind of errors, unfortunately
the strings are extracted by xgettext which we can't easily patch for our
purposes (say to emit warnings about
"word"
"another word"
or
"word "
" another word"
or for this trailing whitespace (this one could be done even on gcc.pot itself
by looking for ' "\nmsgstr', but unfortunately we have various cases where
we intentionally do want those: one category is usually when it ends with
": ", like:
msgid "invalid 'asm': "
msgstr ""
(many cases), but there are even
"Go ahead? (y or n) "
msgstr ""
or
msgid "The following options are specific to just the language "
msgstr ""
"%s\tcompiled by GNU C version %s, "
msgstr ""
msgid "vtable for "
msgstr ""
msgid "%r%s:%d:%d:%R   "
msgstr ""
etc., so it is hard to do this programmatically, unless we had some white
list.

2019-04-09  Jakub Jelinek  <jakub@redhat.com>

	PR translation/90011
	* ipa-devirt.c (compare_virtual_tables): Remove two trailing spaces
	from diagnostics.
	* config/arm/freebsd.h (LINK_SPEC): Remove trailing space from -p
	diagnostics.
	* config/riscv/freebsd.h (LINK_SPEC): Likewise.
	* config/aarch64/aarch64-freebsd.h (FBSD_TARGET_LINK_SPEC): Likewise.
	* config/darwin.h (DRIVER_SELF_SPECS, ASM_FINAL_SPEC): Remove
	trailing space from -gsplit-dwarf diagnostics.


	Jakub

Comments

Richard Biener April 9, 2019, 10:22 a.m. UTC | #1 
On Tue, 9 Apr 2019, Jakub Jelinek wrote:

> Hi!
> 
> Several further spots with trailing whitespace, only bootstrapped/regtested
> on x86_64-linux and i686-linux (so the ipa-devirt.c change is covered),
> the rest is just by eyeballing gcc.pot.
> 
> Ok for trunk?

OK.

Richard.

> I wonder where and how we could check for this kind of errors, unfortunately
> the strings are extracted by xgettext which we can't easily patch for our
> purposes (say to emit warnings about
> "word"
> "another word"
> or
> "word "
> " another word"
> or for this trailing whitespace (this one could be done even on gcc.pot itself
> by looking for ' "\nmsgstr', but unfortunately we have various cases where
> we intentionally do want those: one category is usually when it ends with
> ": ", like:
> msgid "invalid 'asm': "
> msgstr ""
> (many cases), but there are even
> "Go ahead? (y or n) "
> msgstr ""
> or
> msgid "The following options are specific to just the language "
> msgstr ""
> "%s\tcompiled by GNU C version %s, "
> msgstr ""
> msgid "vtable for "
> msgstr ""
> msgid "%r%s:%d:%d:%R   "
> msgstr ""
> etc., so it is hard to do this programmatically, unless we had some white
> list.
> 
> 2019-04-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR translation/90011
> 	* ipa-devirt.c (compare_virtual_tables): Remove two trailing spaces
> 	from diagnostics.
> 	* config/arm/freebsd.h (LINK_SPEC): Remove trailing space from -p
> 	diagnostics.
> 	* config/riscv/freebsd.h (LINK_SPEC): Likewise.
> 	* config/aarch64/aarch64-freebsd.h (FBSD_TARGET_LINK_SPEC): Likewise.
> 	* config/darwin.h (DRIVER_SELF_SPECS, ASM_FINAL_SPEC): Remove
> 	trailing space from -gsplit-dwarf diagnostics.
> 
> --- gcc/ipa-devirt.c.jj	2019-03-08 11:52:17.000000000 +0100
> +++ gcc/ipa-devirt.c	2019-04-08 21:31:32.903689600 +0200
> @@ -874,7 +874,7 @@ compare_virtual_tables (varpool_node *pr
>  				(TYPE_NAME (DECL_CONTEXT (vtable->decl))),
>  			      OPT_Wodr,
>  			      "virtual table of type %qD violates "
> -			      "one definition rule  ",
> +			      "one definition rule",
>  			      DECL_CONTEXT (vtable->decl)))
>  		{
>  		  inform (DECL_SOURCE_LOCATION
> --- gcc/config/arm/freebsd.h.jj	2019-01-01 12:37:28.089795586 +0100
> +++ gcc/config/arm/freebsd.h	2019-04-08 21:26:40.917347492 +0200
> @@ -46,7 +46,7 @@
>  
>  #undef	LINK_SPEC
>  #define LINK_SPEC "							\
> -  %{p:%nconsider using `-pg' instead of `-p' with gprof (1) }		\
> +  %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}		\
>    %{v:-V}								\
>    %{assert*} %{R*} %{rpath*} %{defsym*}					\
>    %{shared:-Bshareable %{h*} %{soname*}}				\
> --- gcc/config/riscv/freebsd.h.jj	2019-01-01 12:37:30.086762821 +0100
> +++ gcc/config/riscv/freebsd.h	2019-04-08 21:25:48.212188263 +0200
> @@ -41,7 +41,7 @@ along with GCC; see the file COPYING3.
>  #undef LINK_SPEC
>  #define LINK_SPEC "						\
>    -melf" XLEN_SPEC "lriscv					\
> -  %{p:%nconsider using `-pg' instead of `-p' with gprof (1) }	\
> +  %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}	\
>    %{v:-V}							\
>    %{assert*} %{R*} %{rpath*} %{defsym*}				\
>    %{shared:-Bshareable %{h*} %{soname*}}			\
> --- gcc/config/aarch64/aarch64-freebsd.h.jj	2019-01-01 12:37:38.460625430 +0100
> +++ gcc/config/aarch64/aarch64-freebsd.h	2019-04-08 21:26:16.311740011 +0200
> @@ -34,7 +34,7 @@
>  
>  #undef  FBSD_TARGET_LINK_SPEC
>  #define FBSD_TARGET_LINK_SPEC "                                 \
> -    %{p:%nconsider using `-pg' instead of `-p' with gprof (1) } \
> +    %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}  \
>      %{v:-V}                                                     \
>      %{assert*} %{R*} %{rpath*} %{defsym*}                       \
>      %{shared:-Bshareable %{h*} %{soname*}}                      \
> --- gcc/config/darwin.h.jj	2019-01-01 12:37:22.137893242 +0100
> +++ gcc/config/darwin.h	2019-04-08 21:25:15.260713922 +0200
> @@ -123,7 +123,7 @@ extern GTY(()) int darwin_ms_struct;
>    "%{gused:-g -feliminate-unused-debug-symbols} %<gused",	\
>    "%{fapple-kext|mkernel:-static}",				\
>    "%{shared:-Zdynamiclib} %<shared",                            \
> -  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform } \
> +  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform} \
>       %<gsplit-dwarf"
>  
>  #define DARWIN_CC1_SPEC							\
> @@ -424,7 +424,7 @@ extern GTY(()) int darwin_ms_struct;
>  
>  #define ASM_DEBUG_SPEC  "%{g*:%{%:debug-level-gt(0):%{!gdwarf*:--gstabs}}}"
>  #define ASM_FINAL_SPEC \
> -  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform } %<gsplit-dwarf"
> +  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform} %<gsplit-dwarf"
>  
>  /* We still allow output of STABS if the assembler supports it.  */
>  #ifdef HAVE_AS_STABS_DIRECTIVE
> 
> 	Jakub
>
David Malcolm April 9, 2019, 2:01 p.m. UTC | #2 
On Tue, 2019-04-09 at 09:21 +0200, Jakub Jelinek wrote:
> Hi!
> 
> Several further spots with trailing whitespace, only
> bootstrapped/regtested
> on x86_64-linux and i686-linux (so the ipa-devirt.c change is
> covered),
> the rest is just by eyeballing gcc.pot.
> 
> Ok for trunk?
> 
> I wonder where and how we could check for this kind of errors,
> unfortunately
> the strings are extracted by xgettext which we can't easily patch for
> our
> purposes (say to emit warnings about
> "word"
> "another word"
> or
> "word "
> " another word"
> or for this trailing whitespace (this one could be done even on
> gcc.pot itself
> by looking for ' "\nmsgstr', but unfortunately we have various cases
> where
> we intentionally do want those: one category is usually when it ends
> with
> ": ", like:
> msgid "invalid 'asm': "
> msgstr ""
> (many cases), but there are even
> "Go ahead? (y or n) "
> msgstr ""
> or
> msgid "The following options are specific to just the language "
> msgstr ""
> "%s\tcompiled by GNU C version %s, "
> msgstr ""
> msgid "vtable for "
> msgstr ""
> msgid "%r%s:%d:%d:%R   "
> msgstr ""
> etc., so it is hard to do this programmatically, unless we had some
> white
> list.


Thinking aloud, maybe c-format.c could check for some of
these?  (provided they're at a suitable callsite).

We're already statically checking for valid format codes/types for
strings at callsites of decls with ATTRIBUTE_GCC_DIAG; maybe that test
could also check for things like trailing whitespace within the string?

Wouldn't help for the LINK_SPEC things, though.

Dave

[...snip...]
David Malcolm April 9, 2019, 2:05 p.m. UTC | #3 
On Tue, 2019-04-09 at 10:01 -0400, David Malcolm wrote:
> On Tue, 2019-04-09 at 09:21 +0200, Jakub Jelinek wrote:
> > Hi!
> > 
> > Several further spots with trailing whitespace, only
> > bootstrapped/regtested
> > on x86_64-linux and i686-linux (so the ipa-devirt.c change is
> > covered),
> > the rest is just by eyeballing gcc.pot.
> > 
> > Ok for trunk?
> > 
> > I wonder where and how we could check for this kind of errors,
> > unfortunately
> > the strings are extracted by xgettext which we can't easily patch
> > for
> > our
> > purposes (say to emit warnings about
> > "word"
> > "another word"
> > or
> > "word "
> > " another word"
> > or for this trailing whitespace (this one could be done even on
> > gcc.pot itself
> > by looking for ' "\nmsgstr', but unfortunately we have various
> > cases
> > where
> > we intentionally do want those: one category is usually when it
> > ends
> > with
> > ": ", like:
> > msgid "invalid 'asm': "
> > msgstr ""
> > (many cases), but there are even
> > "Go ahead? (y or n) "
> > msgstr ""
> > or
> > msgid "The following options are specific to just the language "
> > msgstr ""
> > "%s\tcompiled by GNU C version %s, "
> > msgstr ""
> > msgid "vtable for "
> > msgstr ""
> > msgid "%r%s:%d:%d:%R   "
> > msgstr ""
> > etc., so it is hard to do this programmatically, unless we had some
> > white
> > list.
> 
> 
> Thinking aloud, maybe c-format.c could check for some of
> these?  (provided they're at a suitable callsite).
> 
> We're already statically checking for valid format codes/types for
> strings at callsites of decls with ATTRIBUTE_GCC_DIAG; maybe that
> test
> could also check for things like trailing whitespace within the
> string?
> 
> Wouldn't help for the LINK_SPEC things, though.

FWIW, I *think* the substring location stuff retains enough information
for such an extension of c-format.c to detect bad concatenation cases
like:

"word"
"another word"

> 
> Dave
> 
> [...snip...]
Jakub Jelinek April 9, 2019, 2:17 p.m. UTC | #4 
On Tue, Apr 09, 2019 at 10:05:00AM -0400, David Malcolm wrote:
> > Thinking aloud, maybe c-format.c could check for some of
> > these?  (provided they're at a suitable callsite).
> > 
> > We're already statically checking for valid format codes/types for
> > strings at callsites of decls with ATTRIBUTE_GCC_DIAG; maybe that
> > test
> > could also check for things like trailing whitespace within the
> > string?
> > 
> > Wouldn't help for the LINK_SPEC things, though.
> 
> FWIW, I *think* the substring location stuff retains enough information
> for such an extension of c-format.c to detect bad concatenation cases
> like:
> 
> "word"
> "another word"

I guess you're right.  The question is if something like this would be
useful even for other users.  In that case, as a style warning, it probably
shouldn't be in -Wall or -Wextra (talking about the no or double spaces on
string concatenations from separate lines).
For the no period at the end of diagnostics it would need to be keyed on
gcc-internal-format only of course.

	Jakub
diff mbox series

Patch

--- gcc/ipa-devirt.c.jj	2019-03-08 11:52:17.000000000 +0100
+++ gcc/ipa-devirt.c	2019-04-08 21:31:32.903689600 +0200
@@ -874,7 +874,7 @@  compare_virtual_tables (varpool_node *pr
 				(TYPE_NAME (DECL_CONTEXT (vtable->decl))),
 			      OPT_Wodr,
 			      "virtual table of type %qD violates "
-			      "one definition rule  ",
+			      "one definition rule",
 			      DECL_CONTEXT (vtable->decl)))
 		{
 		  inform (DECL_SOURCE_LOCATION
--- gcc/config/arm/freebsd.h.jj	2019-01-01 12:37:28.089795586 +0100
+++ gcc/config/arm/freebsd.h	2019-04-08 21:26:40.917347492 +0200
@@ -46,7 +46,7 @@ 
 
 #undef	LINK_SPEC
 #define LINK_SPEC "							\
-  %{p:%nconsider using `-pg' instead of `-p' with gprof (1) }		\
+  %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}		\
   %{v:-V}								\
   %{assert*} %{R*} %{rpath*} %{defsym*}					\
   %{shared:-Bshareable %{h*} %{soname*}}				\
--- gcc/config/riscv/freebsd.h.jj	2019-01-01 12:37:30.086762821 +0100
+++ gcc/config/riscv/freebsd.h	2019-04-08 21:25:48.212188263 +0200
@@ -41,7 +41,7 @@  along with GCC; see the file COPYING3.
 #undef LINK_SPEC
 #define LINK_SPEC "						\
   -melf" XLEN_SPEC "lriscv					\
-  %{p:%nconsider using `-pg' instead of `-p' with gprof (1) }	\
+  %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}	\
   %{v:-V}							\
   %{assert*} %{R*} %{rpath*} %{defsym*}				\
   %{shared:-Bshareable %{h*} %{soname*}}			\
--- gcc/config/aarch64/aarch64-freebsd.h.jj	2019-01-01 12:37:38.460625430 +0100
+++ gcc/config/aarch64/aarch64-freebsd.h	2019-04-08 21:26:16.311740011 +0200
@@ -34,7 +34,7 @@ 
 
 #undef  FBSD_TARGET_LINK_SPEC
 #define FBSD_TARGET_LINK_SPEC "                                 \
-    %{p:%nconsider using `-pg' instead of `-p' with gprof (1) } \
+    %{p:%nconsider using `-pg' instead of `-p' with gprof (1)}  \
     %{v:-V}                                                     \
     %{assert*} %{R*} %{rpath*} %{defsym*}                       \
     %{shared:-Bshareable %{h*} %{soname*}}                      \
--- gcc/config/darwin.h.jj	2019-01-01 12:37:22.137893242 +0100
+++ gcc/config/darwin.h	2019-04-08 21:25:15.260713922 +0200
@@ -123,7 +123,7 @@  extern GTY(()) int darwin_ms_struct;
   "%{gused:-g -feliminate-unused-debug-symbols} %<gused",	\
   "%{fapple-kext|mkernel:-static}",				\
   "%{shared:-Zdynamiclib} %<shared",                            \
-  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform } \
+  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform} \
      %<gsplit-dwarf"
 
 #define DARWIN_CC1_SPEC							\
@@ -424,7 +424,7 @@  extern GTY(()) int darwin_ms_struct;
 
 #define ASM_DEBUG_SPEC  "%{g*:%{%:debug-level-gt(0):%{!gdwarf*:--gstabs}}}"
 #define ASM_FINAL_SPEC \
-  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform } %<gsplit-dwarf"
+  "%{gsplit-dwarf:%ngsplit-dwarf is not supported on this platform} %<gsplit-dwarf"
 
 /* We still allow output of STABS if the assembler supports it.  */
 #ifdef HAVE_AS_STABS_DIRECTIVE