diff mbox

[1/7] Add missing punctuation to message (PR driver/79875)

Message ID 1489081529-22256-2-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm March 9, 2017, 5:45 p.m. UTC
gcc/ChangeLog:
	PR driver/79875
	* opts.c (parse_sanitizer_options): Add missing question mark to
	"did you mean" message.
---
 gcc/opts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Martin Sebor March 10, 2017, 4:12 a.m. UTC | #1
On 03/09/2017 10:45 AM, David Malcolm wrote:
> gcc/ChangeLog:
> 	PR driver/79875
> 	* opts.c (parse_sanitizer_options): Add missing question mark to
> 	"did you mean" message.
> ---
>  gcc/opts.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 8274fab..6ea57af 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1640,7 +1640,7 @@ parse_sanitizer_options (const char *p, location_t loc, int scode,
>  	  if (hint)
>  	    error_at (loc,
>  		      "unrecognized argument to -f%ssanitize%s= option: %q.*s;"
> -		      " did you mean %qs",
> +		      " did you mean %qs?",

I have just an observation/question here for future consideration.
If this sort of diagnostic is common (I count 23 instances of it)
or if it is expected to become common, would it make sense to add
a directive for it to the pretty printer to ensure consistency?
I.e., to automatically prepend "; did you mean" and append the
"?" to the new directive?

My search shows the following forms:

1) Space before "?"
gcc/c-family/c-common.c: "%<<<%> in boolean context, did you mean %<<%> ?");

2) No question mark at the end:
gcc/git/gcc/opts.c: "unrecognized argument to -f%ssanitize%s= option: 
%q.*s;"
		      " did you mean %qs",
		      value ? "" : "no-",

3) Missing space after semicolon:

gcc/c/c-decl.c: G_("implicit declaration of function %qE;did you mean 
%qs?"),

4) Explicit quotes:
gcc/gensupport.c: message_at (loc, "(did you mean \"%s\"?)",

(though this won't be fixed by the suggested directive).

Martin
Jakub Jelinek March 10, 2017, 6:18 a.m. UTC | #2
On Thu, Mar 09, 2017 at 12:45:23PM -0500, David Malcolm wrote:
> gcc/ChangeLog:
> 	PR driver/79875
> 	* opts.c (parse_sanitizer_options): Add missing question mark to
> 	"did you mean" message.
> ---
>  gcc/opts.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 8274fab..6ea57af 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1640,7 +1640,7 @@ parse_sanitizer_options (const char *p, location_t loc, int scode,
>  	  if (hint)
>  	    error_at (loc,
>  		      "unrecognized argument to -f%ssanitize%s= option: %q.*s;"
> -		      " did you mean %qs",
> +		      " did you mean %qs?",
>  		      value ? "" : "no-",
>  		      code == OPT_fsanitize_ ? "" : "-recover",
>  		      (int) len, p, hint);

Ok, thanks.

	Jakub
David Malcolm March 10, 2017, 7:46 p.m. UTC | #3
On Thu, 2017-03-09 at 21:12 -0700, Martin Sebor wrote:
> On 03/09/2017 10:45 AM, David Malcolm wrote:
> > gcc/ChangeLog:
> > 	PR driver/79875
> > 	* opts.c (parse_sanitizer_options): Add missing question mark
> > to
> > 	"did you mean" message.
> > ---
> >  gcc/opts.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/gcc/opts.c b/gcc/opts.c
> > index 8274fab..6ea57af 100644
> > --- a/gcc/opts.c
> > +++ b/gcc/opts.c
> > @@ -1640,7 +1640,7 @@ parse_sanitizer_options (const char *p,
> > location_t loc, int scode,
> >  	  if (hint)
> >  	    error_at (loc,
> >  		      "unrecognized argument to -f%ssanitize%s=
> > option: %q.*s;"
> > -		      " did you mean %qs",
> > +		      " did you mean %qs?",
> 
> I have just an observation/question here for future consideration.
> If this sort of diagnostic is common (I count 23 instances of it)
> or if it is expected to become common, would it make sense to add
> a directive for it to the pretty printer to ensure consistency?
> I.e., to automatically prepend "; did you mean" and append the
> "?" to the new directive?

Interesting idea.  The "did you mean" messages tend to be of the form:

  if (hint)
    error ("SOME MESSAGE; did you mean SOME_FORMAT_ARG?",
           ARGS, TO, MESSAGE, HINT_THAT_MATCHES_FORMAT_ARG);
  else
    error ("SOME MESSAGE",
           ARGS, TO, MESSAGE);

which is kind of a pain due to the duplication.  It might be nicer to
eliminate the repetition by making the hint optional, replacing the
conditional with a new directive (e.g. %H):

  error ("SOME MESSAGE%H",
         ARGS, TO, MESSAGE, HINT);

where %H (or whatever) prepends the "; did you mean <FORMATTED_HINT>?"
if HINT is non-NULL, and doesn't if HINT is NULL.

Is this amenable to translation?

(I'm not sure if %H is already in use).

That said, I don't know how many more of these hints we'll add; I think
we've implemented all of the obvious ones now, so this might be over
-engineering at this point.

Some warts here, in addition to the ones you note below:

* it's usually %qs for the hint (11 places), but it's sometimes %qE (3
places), and in one place (gcc.c) it's "%<-%s%>".

> My search shows the following forms:
> 
> 1) Space before "?"
> gcc/c-family/c-common.c: "%<<<%> in boolean context, did you mean
> %<<%> ?");

That extra space feels like an error: the punctuation is quoted, so
it's not ambiguous.

> 2) No question mark at the end:
> gcc/git/gcc/opts.c: "unrecognized argument to -f%ssanitize%s= option:
> %q.*s;"
> 		      " did you mean %qs",
> 		      value ? "" : "no-",

Fixed by the patch.

> 3) Missing space after semicolon:
> 
> gcc/c/c-decl.c: G_("implicit declaration of function %qE;did you mean
> %qs?"),

I'd view that as a bug.

> 4) Explicit quotes:
> gcc/gensupport.c: message_at (loc, "(did you mean \"%s\"?)",
> 
> (though this won't be fixed by the suggested directive).

Indeed, this one ultimately uses vfprintf.

> Martin
Roland Illig March 11, 2017, 10:28 a.m. UTC | #4
Am 10.03.2017 um 05:12 schrieb Martin Sebor:
> I have just an observation/question here for future consideration.
> If this sort of diagnostic is common (I count 23 instances of it)
> or if it is expected to become common, would it make sense to add
> a directive for it to the pretty printer to ensure consistency?
> I.e., to automatically prepend "; did you mean" and append the
> "?" to the new directive?

As a translator, I'm not too concerned about this one, although it looks
very similar to the repeated "%qs requires %qs", which often appears for
command line options.

My main argument is that the "did you mean" is not just a name for a
thing, but it is part of the sentence grammar. To me that feels like too
much magic for a rarely used case (compared to %D or %qE or %qT, which
appear so often that I'm now very familiar with them).

Also, when adding a placeholder like %H for it, there would be no
surrounding whitespace, similar to %K. That confused me as a translator
when I first saw it, since it is not nicely documented; neither in the
Gettext manual
(https://www.gnu.org/software/gettext/manual/gettext.html#gcc_002dinternal_002dformat)
nor in the GCC files implementing them. There should be at least some
examples of what each placeholder typically expands to.

Roland
Bernhard Reutner-Fischer March 14, 2017, 9:57 a.m. UTC | #5
On 11 March 2017 11:28:46 CET, Roland Illig <roland.illig@gmx.de> wrote:
>Am 10.03.2017 um 05:12 schrieb Martin Sebor:
>> I have just an observation/question here for future consideration.
>> If this sort of diagnostic is common (I count 23 instances of it)
>> or if it is expected to become common, would it make sense to add
>> a directive for it to the pretty printer to ensure consistency?
>> I.e., to automatically prepend "; did you mean" and append the
>> "?" to the new directive?
>
>As a translator, I'm not too concerned about this one, although it
>looks
>very similar to the repeated "%qs requires %qs", which often appears
>for
>command line options.
>
>My main argument is that the "did you mean" is not just a name for a
>thing, but it is part of the sentence grammar. To me that feels like
>too
>much magic for a rarely used case (compared to %D or %qE or %qT, which
>appear so often that I'm now very familiar with them).
>
>Also, when adding a placeholder like %H for it, there would be no
>surrounding whitespace, similar to %K. That confused me as a translator
>when I first saw it, since it is not nicely documented; neither in the
>Gettext manual
>(https://www.gnu.org/software/gettext/manual/gettext.html#gcc_002dinternal_002dformat)
>nor in the GCC files implementing them. There should be at least some
>examples of what each placeholder typically expands to.

I think I suggested this a when the hints were introduced but Joseph declined the idea back then, FYI.

thanks,
diff mbox

Patch

diff --git a/gcc/opts.c b/gcc/opts.c
index 8274fab..6ea57af 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1640,7 +1640,7 @@  parse_sanitizer_options (const char *p, location_t loc, int scode,
 	  if (hint)
 	    error_at (loc,
 		      "unrecognized argument to -f%ssanitize%s= option: %q.*s;"
-		      " did you mean %qs",
+		      " did you mean %qs?",
 		      value ? "" : "no-",
 		      code == OPT_fsanitize_ ? "" : "-recover",
 		      (int) len, p, hint);