[5/12] fix diagnostic quoting/spelling in c-family

Message ID 5219f2c3-3ba3-e231-6897-d419032f54bd@gmail.com
State New
Headers show
Series
  • detect quoting and punctuation problems in diagnostics
Related show

Commit Message

Martin Sebor May 14, 2019, 9:32 p.m.
The attached patch fixes quoting, spelling, and other formatting
issues in diagnostics issued from files in the c-family/ directory
and pointed out by the -Wformat-diag warning.

Martin

Comments

Jeff Law May 16, 2019, 7:21 p.m. | #1
On 5/14/19 3:32 PM, Martin Sebor wrote:
> The attached patch fixes quoting, spelling, and other formatting

> issues in diagnostics issued from files in the c-family/ directory

> and pointed out by the -Wformat-diag warning.

> 

> Martin

> 

> gcc-wformat-diag-c-family.diff

> 

> gcc/c-family/ChangeLog:

> 

> 	* c-attribs.c (handle_no_sanitize_attribute): Quote identifiers,

> 	keywords, operators, and types in diagnostics.

> 	(handle_scalar_storage_order_attribute): Same.

> 	(handle_mode_attribute): Same.

> 	(handle_visibility_attribute): Same.

> 	(handle_assume_aligned_attribute): Same.

> 	(handle_no_split_stack_attribute): Same.

> 	* c-common.c (shorten_compare): Same.

> 	(c_common_truthvalue_conversion): Same.

> 	(cb_get_source_date_epoch): Same.

> 	* c-common.h (GCC_DIAG_STYLE): Adjust.

> 	(GCC_DIAG_RAW_STYLE): New macro.

> 	* c-lex.c (cb_def_pragma): Quote keywords, operators, and types

> 	in diagnostics.

> 	(interpret_float): Same.

> 	* c-omp.c (c_finish_omp_for): Same.

> 	* c-opts.c (c_common_post_options): Same.

> 	* c-pch.c (c_common_pch_pragma): Same.

> 	* c-pragma.c (pop_alignment): Same.

> 	(handle_pragma_pack): Same.

> 	(apply_pragma_weak): Same.

> 	(handle_pragma_weak): Same.

> 	(handle_pragma_scalar_storage_order): Same.

> 	(handle_pragma_redefine_extname): Same.

> 	(add_to_renaming_pragma_list): Same.

> 	(maybe_apply_renaming_pragma): Same.

> 	(push_visibility): Same.

> 	(handle_pragma_visibility): Same.

> 	(handle_pragma_optimize): Same.

> 	(handle_pragma_message): Same.

> 	* c-warn.c (warn_for_omitted_condop): Same.

> 	(lvalue_error): Same.

OK
jeff
Joseph Myers May 16, 2019, 11:22 p.m. | #2
On Tue, 14 May 2019, Martin Sebor wrote:

> The attached patch fixes quoting, spelling, and other formatting

> issues in diagnostics issued from files in the c-family/ directory

> and pointed out by the -Wformat-diag warning.


Some of the changes in this patch are questionable.  The diagnostics for 
attribute scalar_storage_order and visibility arguments use \" because the 
argument is a string constant not an identifier.  So making those use %qs 
makes the diagnostics misleading, by suggesting an attribute argument is 
used that is not in fact valid for that attribute.

-- 
Joseph S. Myers
joseph@codesourcery.com
Martin Sebor May 17, 2019, 12:40 a.m. | #3
On 5/16/19 5:22 PM, Joseph Myers wrote:
> On Tue, 14 May 2019, Martin Sebor wrote:

> 

>> The attached patch fixes quoting, spelling, and other formatting

>> issues in diagnostics issued from files in the c-family/ directory

>> and pointed out by the -Wformat-diag warning.

> 

> Some of the changes in this patch are questionable.  The diagnostics for

> attribute scalar_storage_order and visibility arguments use \" because the

> argument is a string constant not an identifier.  So making those use %qs

> makes the diagnostics misleading, by suggesting an attribute argument is

> used that is not in fact valid for that attribute.


Hmm, yes.  I introduced it elsewhere as well in some of my prior
changes, and it existed even before then in handle_visibility_attribute:

     error ("%qD was declared %qs which implies default visibility",
            decl, "dllexport");

There is a way to highlight a string without enclosing it in both
single and double quotes:

     error ("attribute %qE argument must be one of %r%s%R or %r%s%R",
            name, "locus", "\"big-endian\"",
            "locus", "\"little-endian\"");

It's not pretty but it does the job.  Unless you know of some other
trick I'll go with it and fix up the existing mistakes the same way
in a followup commit.

Martin
David Malcolm May 17, 2019, 1:43 p.m. | #4
On Thu, 2019-05-16 at 18:40 -0600, Martin Sebor wrote:
> On 5/16/19 5:22 PM, Joseph Myers wrote:

> > On Tue, 14 May 2019, Martin Sebor wrote:

> > 

> > > The attached patch fixes quoting, spelling, and other formatting

> > > issues in diagnostics issued from files in the c-family/

> > > directory

> > > and pointed out by the -Wformat-diag warning.

> > 

> > Some of the changes in this patch are questionable.  The

> > diagnostics for

> > attribute scalar_storage_order and visibility arguments use \"

> > because the

> > argument is a string constant not an identifier.  So making those

> > use %qs

> > makes the diagnostics misleading, by suggesting an attribute

> > argument is

> > used that is not in fact valid for that attribute.

> 

> Hmm, yes.  I introduced it elsewhere as well in some of my prior

> changes, and it existed even before then in

> handle_visibility_attribute:

> 

>      error ("%qD was declared %qs which implies default visibility",

>             decl, "dllexport");

> 

> There is a way to highlight a string without enclosing it in both

> single and double quotes:

> 

>      error ("attribute %qE argument must be one of %r%s%R or %r%s%R",

>             name, "locus", "\"big-endian\"",

>             "locus", "\"little-endian\"");

> 

> It's not pretty but it does the job.


Interesting idea, but I'm not sure if it's the right way to go here.

Do we have other examples of highlighting strings within a diagnostic?

The colorization typically doesn't show up in compilation logs.  It's
also not easy to test for in DejaGnu (but I can think of some ways to
make that easier).

>  Unless you know of some other

> trick I'll go with it and fix up the existing mistakes the same way

> in a followup commit.


Please can we focus this discussion on what it ought to look like from
the end-user's point of view? (rather than on our implementation
details)

Can you give a concrete example of some source that triggers this
diagnostic, what the current output is, and what the output given the
current candidate patch is?

(i.e. what does it look like to the end-user? what are our ideas for
what should it look like?)

[this may be lack of coffee on my part, but I find it easier to think
and talk about actual input/output examples, rather than diagnostic API
calls and DejaGnu testcases, and let the former lead the latter]

Thanks
Dave
Martin Sebor May 17, 2019, 3:02 p.m. | #5
On 5/17/19 7:43 AM, David Malcolm wrote:
> On Thu, 2019-05-16 at 18:40 -0600, Martin Sebor wrote:

>> On 5/16/19 5:22 PM, Joseph Myers wrote:

>>> On Tue, 14 May 2019, Martin Sebor wrote:

>>>

>>>> The attached patch fixes quoting, spelling, and other formatting

>>>> issues in diagnostics issued from files in the c-family/

>>>> directory

>>>> and pointed out by the -Wformat-diag warning.

>>>

>>> Some of the changes in this patch are questionable.  The

>>> diagnostics for

>>> attribute scalar_storage_order and visibility arguments use \"

>>> because the

>>> argument is a string constant not an identifier.  So making those

>>> use %qs

>>> makes the diagnostics misleading, by suggesting an attribute

>>> argument is

>>> used that is not in fact valid for that attribute.

>>

>> Hmm, yes.  I introduced it elsewhere as well in some of my prior

>> changes, and it existed even before then in

>> handle_visibility_attribute:

>>

>>       error ("%qD was declared %qs which implies default visibility",

>>              decl, "dllexport");

>>

>> There is a way to highlight a string without enclosing it in both

>> single and double quotes:

>>

>>       error ("attribute %qE argument must be one of %r%s%R or %r%s%R",

>>              name, "locus", "\"big-endian\"",

>>              "locus", "\"little-endian\"");

>>

>> It's not pretty but it does the job.

> 

> Interesting idea, but I'm not sure if it's the right way to go here.

> 

> Do we have other examples of highlighting strings within a diagnostic?

> 

> The colorization typically doesn't show up in compilation logs.  It's

> also not easy to test for in DejaGnu (but I can think of some ways to

> make that easier).

> 

>>   Unless you know of some other

>> trick I'll go with it and fix up the existing mistakes the same way

>> in a followup commit.

> 

> Please can we focus this discussion on what it ought to look like from

> the end-user's point of view? (rather than on our implementation

> details)

> 

> Can you give a concrete example of some source that triggers this

> diagnostic, what the current output is, and what the output given the

> current candidate patch is?

> 

> (i.e. what does it look like to the end-user? what are our ideas for

> what should it look like?)

> 

> [this may be lack of coffee on my part, but I find it easier to think

> and talk about actual input/output examples, rather than diagnostic API

> calls and DejaGnu testcases, and let the former lead the latter]


Here's an example:

   struct __attribute__ ((scalar_storage_order ("big endian"))) S { int 
i; };

and here's the output with the latest patch and using %r/%R:

$ gcc -O2 -S -Wall b.c
b.c:1:62: error: attribute ‘scalar_storage_order’ argument must be one 
of "big-endian" or "little-endian"
     1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S 
{ int i; };
       |                                                              ^

The name scalar_storage_order is highlighted (and in single quotes)
and the strings "big-endian" and "little-endian" are just highlighted
(including the quotes, but no single-quotes).  That looks right to
me but YMMV.

What Joseph pointed out is that using %qs to quote big-endian ends
up with either

   error: attribute ‘scalar_storage_order’ argument must be one of 
'big-endian' or 'little-endian'

if the strings are just big-endian and little-endian (i.e., not
including the double-quotes), which suggests to the user they
shouldn't be quoted in the source either, or with

   error: attribute ‘scalar_storage_order’ argument must be one of 
'"big-endian"' or '"little-endian"'

if the strings themselves contain the double-quotes (i.e., are
passed to %qs as "\"big-endian\""...  Single-quoting a string in
double-quotes, although technically correct, looks odd to me.

Unpatched trunk doesn't highlight anything which is what I'm fixing.

Martin
David Malcolm May 17, 2019, 5:16 p.m. | #6
On Fri, 2019-05-17 at 09:02 -0600, Martin Sebor wrote:
> On 5/17/19 7:43 AM, David Malcolm wrote:

> > On Thu, 2019-05-16 at 18:40 -0600, Martin Sebor wrote:

> > > On 5/16/19 5:22 PM, Joseph Myers wrote:

> > > > On Tue, 14 May 2019, Martin Sebor wrote:

> > > > 

> > > > > The attached patch fixes quoting, spelling, and other

> > > > > formatting

> > > > > issues in diagnostics issued from files in the c-family/

> > > > > directory

> > > > > and pointed out by the -Wformat-diag warning.

> > > > 

> > > > Some of the changes in this patch are questionable.  The

> > > > diagnostics for

> > > > attribute scalar_storage_order and visibility arguments use \"

> > > > because the

> > > > argument is a string constant not an identifier.  So making

> > > > those

> > > > use %qs

> > > > makes the diagnostics misleading, by suggesting an attribute

> > > > argument is

> > > > used that is not in fact valid for that attribute.

> > > 

> > > Hmm, yes.  I introduced it elsewhere as well in some of my prior

> > > changes, and it existed even before then in

> > > handle_visibility_attribute:

> > > 

> > >       error ("%qD was declared %qs which implies default

> > > visibility",

> > >              decl, "dllexport");

> > > 

> > > There is a way to highlight a string without enclosing it in both

> > > single and double quotes:

> > > 

> > >       error ("attribute %qE argument must be one of %r%s%R or

> > > %r%s%R",

> > >              name, "locus", "\"big-endian\"",

> > >              "locus", "\"little-endian\"");

> > > 

> > > It's not pretty but it does the job.

> > 

> > Interesting idea, but I'm not sure if it's the right way to go

> > here.

> > 

> > Do we have other examples of highlighting strings within a

> > diagnostic?

> > 

> > The colorization typically doesn't show up in compilation

> > logs.  It's

> > also not easy to test for in DejaGnu (but I can think of some ways

> > to

> > make that easier).

> > 

> > >   Unless you know of some other

> > > trick I'll go with it and fix up the existing mistakes the same

> > > way

> > > in a followup commit.

> > 

> > Please can we focus this discussion on what it ought to look like

> > from

> > the end-user's point of view? (rather than on our implementation

> > details)

> > 

> > Can you give a concrete example of some source that triggers this

> > diagnostic, what the current output is, and what the output given

> > the

> > current candidate patch is?

> > 

> > (i.e. what does it look like to the end-user? what are our ideas

> > for

> > what should it look like?)

> > 

> > [this may be lack of coffee on my part, but I find it easier to

> > think

> > and talk about actual input/output examples, rather than diagnostic

> > API

> > calls and DejaGnu testcases, and let the former lead the latter]

> 

> Here's an example:

> 

>    struct __attribute__ ((scalar_storage_order ("big endian"))) S {

> int 

> i; };

> 

> and here's the output with the latest patch and using %r/%R:

> 

> $ gcc -O2 -S -Wall b.c

> b.c:1:62: error: attribute ‘scalar_storage_order’ argument must be

> one 

> of "big-endian" or "little-endian"

>      1 | struct __attribute__ ((scalar_storage_order ("big endian")))

> S 

> { int i; };

>        |                                                             

>  ^

> 

> The name scalar_storage_order is highlighted (and in single quotes)

> and the strings "big-endian" and "little-endian" are just highlighted

> (including the quotes, but no single-quotes).  That looks right to

> me but YMMV.

> 

> What Joseph pointed out is that using %qs to quote big-endian ends

> up with either

> 

>    error: attribute ‘scalar_storage_order’ argument must be one of 

> 'big-endian' or 'little-endian'

> 

> if the strings are just big-endian and little-endian (i.e., not

> including the double-quotes), which suggests to the user they

> shouldn't be quoted in the source either, or with

> 

>    error: attribute ‘scalar_storage_order’ argument must be one of 

> '"big-endian"' or '"little-endian"'


This was actually the thing I had in the back in my mind when I sent my
last email.

The above uses "'", but the quoting would use "‘" and "’":

   error: attribute ‘scalar_storage_order’ argument must be one of 
   ‘"big-endian"’ or ‘"little-endian"’

> if the strings themselves contain the double-quotes (i.e., are

> passed to %qs as "\"big-endian\""...  


An easy way to implement this would be via:
  "must be one of %<\"%s\"%> or " etc
which would avoid having to manipulate the name.

> Single-quoting a string in

> double-quotes, although technically correct, looks odd to me.


I think it's clearer, though I agree it does look odd.

I think I prefer single-quoting the double-quoted string in that the
user isn't meant to have wrapped
  scalar_storage_order
in double-quotes, but they are meant to have wrapped the argument in
double-quotes.

> Unpatched trunk doesn't highlight anything which is what I'm fixing.


My other thought is: do we have enough location information to offer
fix-it hints.  Then it could be something like:

error: attribute ‘scalar_storage_order’ argument must be
one of ‘"big-endian"’...
   1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S { int i; };
     |                                              ~~~~~~~~~~~~    ^
     |                                              "big-endian"
note: ...or ‘"little-endian"’
   1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S {
int i; };
     |                                              ~~~~~~~~~~~~    ^
     |                                              "little-endian"

Even if we don't have location information, it strikes me that for a
close match like this, some kind of "do you mean" spellcheck-style hint
might be appropriate:

error: attribute ‘scalar_storage_order’ argument must be one of ‘"big-
endian"’  or ‘"little-endian"’; did you mean ‘"big-endian"’?
   1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S {
int i; };
     |                                              ~~~~~~~~~~~~    ^
     |                                              "big-endian"

Or somesuch

Hope this is constructive
Dave
Martin Sebor May 17, 2019, 6:07 p.m. | #7
On 5/17/19 11:16 AM, David Malcolm wrote:
> On Fri, 2019-05-17 at 09:02 -0600, Martin Sebor wrote:

>> On 5/17/19 7:43 AM, David Malcolm wrote:

>>> On Thu, 2019-05-16 at 18:40 -0600, Martin Sebor wrote:

>>>> On 5/16/19 5:22 PM, Joseph Myers wrote:

>>>>> On Tue, 14 May 2019, Martin Sebor wrote:

>>>>>

>>>>>> The attached patch fixes quoting, spelling, and other

>>>>>> formatting

>>>>>> issues in diagnostics issued from files in the c-family/

>>>>>> directory

>>>>>> and pointed out by the -Wformat-diag warning.

>>>>>

>>>>> Some of the changes in this patch are questionable.  The

>>>>> diagnostics for

>>>>> attribute scalar_storage_order and visibility arguments use \"

>>>>> because the

>>>>> argument is a string constant not an identifier.  So making

>>>>> those

>>>>> use %qs

>>>>> makes the diagnostics misleading, by suggesting an attribute

>>>>> argument is

>>>>> used that is not in fact valid for that attribute.

>>>>

>>>> Hmm, yes.  I introduced it elsewhere as well in some of my prior

>>>> changes, and it existed even before then in

>>>> handle_visibility_attribute:

>>>>

>>>>        error ("%qD was declared %qs which implies default

>>>> visibility",

>>>>               decl, "dllexport");

>>>>

>>>> There is a way to highlight a string without enclosing it in both

>>>> single and double quotes:

>>>>

>>>>        error ("attribute %qE argument must be one of %r%s%R or

>>>> %r%s%R",

>>>>               name, "locus", "\"big-endian\"",

>>>>               "locus", "\"little-endian\"");

>>>>

>>>> It's not pretty but it does the job.

>>>

>>> Interesting idea, but I'm not sure if it's the right way to go

>>> here.

>>>

>>> Do we have other examples of highlighting strings within a

>>> diagnostic?

>>>

>>> The colorization typically doesn't show up in compilation

>>> logs.  It's

>>> also not easy to test for in DejaGnu (but I can think of some ways

>>> to

>>> make that easier).

>>>

>>>>    Unless you know of some other

>>>> trick I'll go with it and fix up the existing mistakes the same

>>>> way

>>>> in a followup commit.

>>>

>>> Please can we focus this discussion on what it ought to look like

>>> from

>>> the end-user's point of view? (rather than on our implementation

>>> details)

>>>

>>> Can you give a concrete example of some source that triggers this

>>> diagnostic, what the current output is, and what the output given

>>> the

>>> current candidate patch is?

>>>

>>> (i.e. what does it look like to the end-user? what are our ideas

>>> for

>>> what should it look like?)

>>>

>>> [this may be lack of coffee on my part, but I find it easier to

>>> think

>>> and talk about actual input/output examples, rather than diagnostic

>>> API

>>> calls and DejaGnu testcases, and let the former lead the latter]

>>

>> Here's an example:

>>

>>     struct __attribute__ ((scalar_storage_order ("big endian"))) S {

>> int

>> i; };

>>

>> and here's the output with the latest patch and using %r/%R:

>>

>> $ gcc -O2 -S -Wall b.c

>> b.c:1:62: error: attribute ‘scalar_storage_order’ argument must be

>> one

>> of "big-endian" or "little-endian"

>>       1 | struct __attribute__ ((scalar_storage_order ("big endian")))

>> S

>> { int i; };

>>         |

>>   ^

>>

>> The name scalar_storage_order is highlighted (and in single quotes)

>> and the strings "big-endian" and "little-endian" are just highlighted

>> (including the quotes, but no single-quotes).  That looks right to

>> me but YMMV.

>>

>> What Joseph pointed out is that using %qs to quote big-endian ends

>> up with either

>>

>>     error: attribute ‘scalar_storage_order’ argument must be one of

>> 'big-endian' or 'little-endian'

>>

>> if the strings are just big-endian and little-endian (i.e., not

>> including the double-quotes), which suggests to the user they

>> shouldn't be quoted in the source either, or with

>>

>>     error: attribute ‘scalar_storage_order’ argument must be one of

>> '"big-endian"' or '"little-endian"'

> 

> This was actually the thing I had in the back in my mind when I sent my

> last email.

> 

> The above uses "'", but the quoting would use "‘" and "’":


Sure, depending on the locale.  Either way, we end up two pairs
of quotes around the string.

> 

>     error: attribute ‘scalar_storage_order’ argument must be one of

>     ‘"big-endian"’ or ‘"little-endian"’

> 

>> if the strings themselves contain the double-quotes (i.e., are

>> passed to %qs as "\"big-endian\""...

> 

> An easy way to implement this would be via:

>    "must be one of %<\"%s\"%> or " etc

> which would avoid having to manipulate the name.


Yes, but bug 90156 and check-internal-format-escaping.py want to
banish this form so that's what I've implemented in the -Wformat-
diag checker as well.

>> Single-quoting a string in

>> double-quotes, although technically correct, looks odd to me.

> 

> I think it's clearer, though I agree it does look odd.

> 

> I think I prefer single-quoting the double-quoted string in that the

> user isn't meant to have wrapped

>    scalar_storage_order

> in double-quotes, but they are meant to have wrapped the argument in

> double-quotes.

> 

>> Unpatched trunk doesn't highlight anything which is what I'm fixing.

> 

> My other thought is: do we have enough location information to offer

> fix-it hints.  Then it could be something like:


I don't think we do have the location of attribute arguments.  It
would be nice if we did for reasons other than the quoting issue.

> 

> error: attribute ‘scalar_storage_order’ argument must be

> one of ‘"big-endian"’...

>     1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S { int i; };

>       |                                              ~~~~~~~~~~~~    ^

>       |                                              "big-endian"

> note: ...or ‘"little-endian"’

>     1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S {

> int i; };

>       |                                              ~~~~~~~~~~~~    ^

>       |                                              "little-endian"

> 

> Even if we don't have location information, it strikes me that for a

> close match like this, some kind of "do you mean" spellcheck-style hint

> might be appropriate:

> 

> error: attribute ‘scalar_storage_order’ argument must be one of ‘"big-

> endian"’  or ‘"little-endian"’; did you mean ‘"big-endian"’?

>     1 | struct __attribute__ ((scalar_storage_order ("big endian"))) S {

> int i; };

>       |                                              ~~~~~~~~~~~~    ^

>       |                                              "big-endian"

> 

> Or somesuch


Sure.  It's a nice enhancement but it doesn't solve with the problem
of two pairs of quotes.

Martin
David Malcolm May 17, 2019, 6:53 p.m. | #8
On Fri, 2019-05-17 at 12:07 -0600, Martin Sebor wrote:
> On 5/17/19 11:16 AM, David Malcolm wrote:

> > On Fri, 2019-05-17 at 09:02 -0600, Martin Sebor wrote:

> > > On 5/17/19 7:43 AM, David Malcolm wrote:

> > > > On Thu, 2019-05-16 at 18:40 -0600, Martin Sebor wrote:

> > > > > On 5/16/19 5:22 PM, Joseph Myers wrote:

> > > > > > On Tue, 14 May 2019, Martin Sebor wrote:

> > > > > > 

> > > > > > > The attached patch fixes quoting, spelling, and other

> > > > > > > formatting

> > > > > > > issues in diagnostics issued from files in the c-family/

> > > > > > > directory

> > > > > > > and pointed out by the -Wformat-diag warning.

> > > > > > 

> > > > > > Some of the changes in this patch are questionable.  The

> > > > > > diagnostics for

> > > > > > attribute scalar_storage_order and visibility arguments use

> > > > > > \"

> > > > > > because the

> > > > > > argument is a string constant not an identifier.  So making

> > > > > > those

> > > > > > use %qs

> > > > > > makes the diagnostics misleading, by suggesting an

> > > > > > attribute

> > > > > > argument is

> > > > > > used that is not in fact valid for that attribute.

> > > > > 

> > > > > Hmm, yes.  I introduced it elsewhere as well in some of my

> > > > > prior

> > > > > changes, and it existed even before then in

> > > > > handle_visibility_attribute:

> > > > > 

> > > > >        error ("%qD was declared %qs which implies default

> > > > > visibility",

> > > > >               decl, "dllexport");

> > > > > 

> > > > > There is a way to highlight a string without enclosing it in

> > > > > both

> > > > > single and double quotes:

> > > > > 

> > > > >        error ("attribute %qE argument must be one of %r%s%R

> > > > > or

> > > > > %r%s%R",

> > > > >               name, "locus", "\"big-endian\"",

> > > > >               "locus", "\"little-endian\"");

> > > > > 

> > > > > It's not pretty but it does the job.

> > > > 

> > > > Interesting idea, but I'm not sure if it's the right way to go

> > > > here.

> > > > 

> > > > Do we have other examples of highlighting strings within a

> > > > diagnostic?

> > > > 

> > > > The colorization typically doesn't show up in compilation

> > > > logs.  It's

> > > > also not easy to test for in DejaGnu (but I can think of some

> > > > ways

> > > > to

> > > > make that easier).

> > > > 

> > > > >    Unless you know of some other

> > > > > trick I'll go with it and fix up the existing mistakes the

> > > > > same

> > > > > way

> > > > > in a followup commit.

> > > > 

> > > > Please can we focus this discussion on what it ought to look

> > > > like

> > > > from

> > > > the end-user's point of view? (rather than on our

> > > > implementation

> > > > details)

> > > > 

> > > > Can you give a concrete example of some source that triggers

> > > > this

> > > > diagnostic, what the current output is, and what the output

> > > > given

> > > > the

> > > > current candidate patch is?

> > > > 

> > > > (i.e. what does it look like to the end-user? what are our

> > > > ideas

> > > > for

> > > > what should it look like?)

> > > > 

> > > > [this may be lack of coffee on my part, but I find it easier to

> > > > think

> > > > and talk about actual input/output examples, rather than

> > > > diagnostic

> > > > API

> > > > calls and DejaGnu testcases, and let the former lead the

> > > > latter]

> > > 

> > > Here's an example:

> > > 

> > >     struct __attribute__ ((scalar_storage_order ("big endian")))

> > > S {

> > > int

> > > i; };

> > > 

> > > and here's the output with the latest patch and using %r/%R:

> > > 

> > > $ gcc -O2 -S -Wall b.c

> > > b.c:1:62: error: attribute ‘scalar_storage_order’ argument must

> > > be

> > > one

> > > of "big-endian" or "little-endian"

> > >       1 | struct __attribute__ ((scalar_storage_order ("big

> > > endian")))

> > > S

> > > { int i; };

> > >         |

> > >   ^

> > > 

> > > The name scalar_storage_order is highlighted (and in single

> > > quotes)

> > > and the strings "big-endian" and "little-endian" are just

> > > highlighted

> > > (including the quotes, but no single-quotes).  That looks right

> > > to

> > > me but YMMV.

> > > 

> > > What Joseph pointed out is that using %qs to quote big-endian

> > > ends

> > > up with either

> > > 

> > >     error: attribute ‘scalar_storage_order’ argument must be one

> > > of

> > > 'big-endian' or 'little-endian'

> > > 

> > > if the strings are just big-endian and little-endian (i.e., not

> > > including the double-quotes), which suggests to the user they

> > > shouldn't be quoted in the source either, or with

> > > 

> > >     error: attribute ‘scalar_storage_order’ argument must be one

> > > of

> > > '"big-endian"' or '"little-endian"'

> > 

> > This was actually the thing I had in the back in my mind when I

> > sent my

> > last email.

> > 

> > The above uses "'", but the quoting would use "‘" and "’":

> 

> Sure, depending on the locale.  Either way, we end up two pairs

> of quotes around the string.

> 

> > 

> >     error: attribute ‘scalar_storage_order’ argument must be one of

> >     ‘"big-endian"’ or ‘"little-endian"’

> > 

> > > if the strings themselves contain the double-quotes (i.e., are

> > > passed to %qs as "\"big-endian\""...

> > 

> > An easy way to implement this would be via:

> >    "must be one of %<\"%s\"%> or " etc

> > which would avoid having to manipulate the name.

> 

> Yes, but bug 90156 and check-internal-format-escaping.py want to

> banish this form so that's what I've implemented in the -Wformat-

> diag checker as well.


Presumably you're referring to bug 90156's:
  "please also add \"%s\" to the forbidden string literals."

The checker presumably could treat %<\"%s\"%>  as an exception to this
rule.  If that form is best here (I think it is), then the checker
should be fixed to allow that; otherwise, aren't we putting the cart
before the horse?

> > > Single-quoting a string in

> > > double-quotes, although technically correct, looks odd to me.

> > 

> > I think it's clearer, though I agree it does look odd.

> > 

> > I think I prefer single-quoting the double-quoted string in that

> > the

> > user isn't meant to have wrapped

> >    scalar_storage_order

> > in double-quotes, but they are meant to have wrapped the argument

> > in

> > double-quotes.

> > 

> > > Unpatched trunk doesn't highlight anything which is what I'm

> > > fixing.

> > 

> > My other thought is: do we have enough location information to

> > offer

> > fix-it hints.  Then it could be something like:

> 

> I don't think we do have the location of attribute arguments.  It

> would be nice if we did for reasons other than the quoting issue.


Yeah.  Fixing that would probably require some changes to how we handle
attributes, though (e.g. passing a location_t around with the
attributes?)

> > 

> > error: attribute ‘scalar_storage_order’ argument must be

> > one of ‘"big-endian"’...

> >     1 | struct __attribute__ ((scalar_storage_order ("big

> > endian"))) S { int i; };

> >       |                                              ~~~~~~~~~~~~  

> >   ^

> >       |                                              "big-endian"

> > note: ...or ‘"little-endian"’

> >     1 | struct __attribute__ ((scalar_storage_order ("big

> > endian"))) S {

> > int i; };

> >       |                                              ~~~~~~~~~~~~  

> >   ^

> >       |                                              "little-

> > endian"

> > 

> > Even if we don't have location information, it strikes me that for

> > a

> > close match like this, some kind of "do you mean" spellcheck-style

> > hint

> > might be appropriate:

> > 

> > error: attribute ‘scalar_storage_order’ argument must be one of

> > ‘"big-

> > endian"’  or ‘"little-endian"’; did you mean ‘"big-endian"’?

> >     1 | struct __attribute__ ((scalar_storage_order ("big

> > endian"))) S {

> > int i; };

> >       |                                              ~~~~~~~~~~~~  

> >   ^

> >       |                                              "big-endian"

> > 

> > Or somesuch

> 

> Sure.  It's a nice enhancement but it doesn't solve with the problem

> of two pairs of quotes.

> 

> Martin
Joseph Myers May 20, 2019, 8 p.m. | #9
Another couple of issues in the change as committed:

> +       GCC_BAD ("%<malformed #pragma weak%>, ignored");


The word "malformed" should be outside the %<%>.

> +Warn about GCC format strings with strings unsuitable for diagnostics..


This help text should end with one '.', not two.

-- 
Joseph S. Myers
joseph@codesourcery.com
Martin Sebor May 20, 2019, 10:08 p.m. | #10
On 5/20/19 2:00 PM, Joseph Myers wrote:
> Another couple of issues in the change as committed:

> 

>> +       GCC_BAD ("%<malformed #pragma weak%>, ignored");

> 

> The word "malformed" should be outside the %<%>.

> 

>> +Warn about GCC format strings with strings unsuitable for diagnostics..

> 

> This help text should end with one '.', not two.

> 


Thanks.  The attached patch fixes that.  I'll commit it as obvious
unless your someone else has concerns/suggestions for changes.

Martin
gcc/c-family/ChangeLog:

	* c-pragma.c (handle_pragma_weak): Adjust quoting in a diagnostic.
	* c.opt (-Wformat-diag): Remove a spurious period.

gcc/testsuite/ChangeLog:

	* gcc.dg/weak/weak-19.c: New test.

Index: gcc/c-family/c-pragma.c
===================================================================
--- gcc/c-family/c-pragma.c	(revision 271441)
+++ gcc/c-family/c-pragma.c	(working copy)
@@ -361,7 +361,7 @@ handle_pragma_weak (cpp_reader * ARG_UNUSED (dummy
   if (t == CPP_EQ)
     {
       if (pragma_lex (&value) != CPP_NAME)
-	GCC_BAD ("%<malformed #pragma weak%>, ignored");
+	GCC_BAD ("malformed %<#pragma weak%>, ignored");
       t = pragma_lex (&x);
     }
   if (t != CPP_EOF)
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 271441)
+++ gcc/c-family/c.opt	(working copy)
@@ -564,7 +564,7 @@ Warn about format strings that contain NUL bytes.
 
 Wformat-diag
 C ObjC C++ ObjC++ Var(warn_format_diag) Warning LangEnabledBy(C ObjC C++ ObjC++,Wformat=, warn_format >= 1, 0)
-Warn about GCC format strings with strings unsuitable for diagnostics..
+Warn about GCC format strings with strings unsuitable for diagnostics.
 
 Wformat-extra-args
 C ObjC C++ ObjC++ Var(warn_format_extra_args) Warning LangEnabledBy(C ObjC C++ ObjC++,Wformat=, warn_format >= 1, 0)
Index: gcc/testsuite/gcc.dg/weak/weak-19.c
===================================================================
--- gcc/testsuite/gcc.dg/weak/weak-19.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/weak/weak-19.c	(working copy)
@@ -0,0 +1,8 @@
+/* { dg-do compile }
+   { dg-require-weak "" } */
+
+/* Verify that the following invalid pragmas are diagnosed.  */
+
+#pragma weak foo = 1   /* { dg-warning "malformed '#pragma weak'" } */
+#pragma weak foo = +   /* { dg-warning "malformed '#pragma weak'" } */
+#pragma weak foo = ?   /* { dg-warning "malformed '#pragma weak'" } */

Patch

gcc/c-family/ChangeLog:

	* c-attribs.c (handle_no_sanitize_attribute): Quote identifiers,
	keywords, operators, and types in diagnostics.
	(handle_scalar_storage_order_attribute): Same.
	(handle_mode_attribute): Same.
	(handle_visibility_attribute): Same.
	(handle_assume_aligned_attribute): Same.
	(handle_no_split_stack_attribute): Same.
	* c-common.c (shorten_compare): Same.
	(c_common_truthvalue_conversion): Same.
	(cb_get_source_date_epoch): Same.
	* c-common.h (GCC_DIAG_STYLE): Adjust.
	(GCC_DIAG_RAW_STYLE): New macro.
	* c-lex.c (cb_def_pragma): Quote keywords, operators, and types
	in diagnostics.
	(interpret_float): Same.
	* c-omp.c (c_finish_omp_for): Same.
	* c-opts.c (c_common_post_options): Same.
	* c-pch.c (c_common_pch_pragma): Same.
	* c-pragma.c (pop_alignment): Same.
	(handle_pragma_pack): Same.
	(apply_pragma_weak): Same.
	(handle_pragma_weak): Same.
	(handle_pragma_scalar_storage_order): Same.
	(handle_pragma_redefine_extname): Same.
	(add_to_renaming_pragma_list): Same.
	(maybe_apply_renaming_pragma): Same.
	(push_visibility): Same.
	(handle_pragma_visibility): Same.
	(handle_pragma_optimize): Same.
	(handle_pragma_message): Same.
	* c-warn.c (warn_for_omitted_condop): Same.
	(lvalue_error): Same.

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 93b210eed9c..60e5d851586 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -891,7 +891,7 @@  handle_no_sanitize_attribute (tree *node, tree name, tree args, int,
       tree id = TREE_VALUE (args);
       if (TREE_CODE (id) != STRING_CST)
 	{
-	  error ("no_sanitize argument not a string");
+	  error ("%qE argument not a string", name);
 	  return NULL_TREE;
 	}
 
@@ -1418,8 +1418,8 @@  handle_scalar_storage_order_attribute (tree *node, tree name, tree args,
 
   if (BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN)
     {
-      error ("scalar_storage_order is not supported because endianness "
-	    "is not uniform");
+      error ("%qE attribute is not supported because endianness is not uniform",
+	     name);
       return NULL_TREE;
     }
 
@@ -1435,8 +1435,8 @@  handle_scalar_storage_order_attribute (tree *node, tree name, tree args,
 	reverse = BYTES_BIG_ENDIAN;
       else
 	{
-	  error ("scalar_storage_order argument must be one of \"big-endian\""
-		 " or \"little-endian\"");
+	  error ("attribute %qE argument must be one of %qs or %qs",
+		 name, "big-endian", "little-endian");
 	  return NULL_TREE;
 	}
 
@@ -1759,9 +1759,9 @@  handle_mode_attribute (tree *node, tree name, tree args,
 	case MODE_VECTOR_ACCUM:
 	case MODE_VECTOR_UACCUM:
 	  warning (OPT_Wattributes, "specifying vector types with "
-		   "__attribute__ ((mode)) is deprecated");
-	  warning (OPT_Wattributes,
-		   "use __attribute__ ((vector_size)) instead");
+		   "%<__attribute__ ((mode))%> is deprecated");
+	  inform (input_location,
+		  "use %<__attribute__ ((vector_size))%> instead");
 	  valid_mode = vector_mode_valid_p (mode);
 	  break;
 
@@ -2671,7 +2671,8 @@  handle_visibility_attribute (tree *node, tree name, tree args,
     vis = VISIBILITY_PROTECTED;
   else
     {
-      error ("visibility argument must be one of \"default\", \"hidden\", \"protected\" or \"internal\"");
+      error ("attribute %qE argument must be one of %qs, %qs, %qs, or %qs",
+	     name, "default", "hidden", "protected", "internal");
       vis = VISIBILITY_DEFAULT;
     }
 
@@ -2935,8 +2936,8 @@  handle_assume_aligned_attribute (tree *node, tree name, tree args, int,
 	  /* The misalignment specified by the second argument
 	     must be non-negative and less than the alignment.  */
 	  warning (OPT_Wattributes,
-		   "%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, ')');
 	  *no_add_attrs = true;
 	  return NULL_TREE;
 	}
@@ -3985,13 +3986,13 @@  handle_no_split_stack_attribute (tree *node, tree name,
    struct attribute_spec.handler.  */
 
 static tree
-handle_returns_nonnull_attribute (tree *node, tree, tree, int,
+handle_returns_nonnull_attribute (tree *node, tree name, tree, int,
 				  bool *no_add_attrs)
 {
   // Even without a prototype we still have a return type we can check.
   if (TREE_CODE (TREE_TYPE (*node)) != POINTER_TYPE)
     {
-      error ("returns_nonnull attribute on a function not returning a pointer");
+      error ("%qE attribute on a function not returning a pointer", name);
       *no_add_attrs = true;
     }
   return NULL_TREE;
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3c2f7a9d1c7..4cb2082959a 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3063,14 +3063,16 @@  shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
 	    case GE_EXPR:
 	      if (warn)
 		warning_at (loc, OPT_Wtype_limits,
-			    "comparison of unsigned expression >= 0 is always true");
+			    "comparison of unsigned expression in %<>= 0%> "
+			    "is always true");
 	      value = truthvalue_true_node;
 	      break;
 
 	    case LT_EXPR:
 	      if (warn)
 		warning_at (loc, OPT_Wtype_limits,
-			    "comparison of unsigned expression < 0 is always false");
+			    "comparison of unsigned expression in %<< 0%> "
+			    "is always false");
 	      value = truthvalue_false_node;
 	      break;
 
@@ -3379,7 +3381,7 @@  c_common_truthvalue_conversion (location_t location, tree expr)
       if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
 	  && !TYPE_UNSIGNED (TREE_TYPE (expr)))
 	warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-		    "%<<<%> in boolean context, did you mean %<<%> ?");
+		    "%<<<%> in boolean context, did you mean %<<%>?");
       break;
 
     case COND_EXPR:
@@ -3395,7 +3397,7 @@  c_common_truthvalue_conversion (location_t location, tree expr)
 	      && (!integer_onep (val1)
 		  || !integer_onep (val2)))
 	    warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-			"?: using integer constants in boolean context, "
+			"%<?:%> using integer constants in boolean context, "
 			"the expression will always evaluate to %<true%>");
 	  else if ((TREE_CODE (val1) == INTEGER_CST
 		    && !integer_zerop (val1)
@@ -3404,7 +3406,7 @@  c_common_truthvalue_conversion (location_t location, tree expr)
 		       && !integer_zerop (val2)
 		       && !integer_onep (val2)))
 	    warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-			"?: using integer constants in boolean context");
+			"%<?:%> using integer constants in boolean context");
 	}
       /* Distribute the conversion into the arms of a COND_EXPR.  */
       if (c_dialect_cxx ())
@@ -8257,9 +8259,9 @@  cb_get_source_date_epoch (cpp_reader *pfile ATTRIBUTE_UNUSED)
   if (errno != 0 || endptr == source_date_epoch || *endptr != '\0'
       || epoch < 0 || epoch > MAX_SOURCE_DATE_EPOCH)
     {
-      error_at (input_location, "environment variable SOURCE_DATE_EPOCH must "
+      error_at (input_location, "environment variable %qs must "
 	        "expand to a non-negative integer less than or equal to %wd",
-		MAX_SOURCE_DATE_EPOCH);
+		"SOURCE_DATE_EPOCH", MAX_SOURCE_DATE_EPOCH);
       return (time_t) -1;
     }
 
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 1cf2cae6395..9d067028416 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -39,7 +39,10 @@  framework extensions, you must include this file before diagnostic-core.h \
 never after.
 #endif
 #ifndef GCC_DIAG_STYLE
-#define GCC_DIAG_STYLE __gcc_cdiag__
+#  define GCC_DIAG_STYLE       __gcc_cdiag__
+#endif
+#ifndef GCC_DIAG_RAW_STYLE
+#  define GCC_DIAG_RAW_STYLE   __gcc_cdiag_raw__
 #endif
 #include "diagnostic-core.h"
 
diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
index 0a368a33a58..851fd704e5d 100644
--- a/gcc/c-family/c-lex.c
+++ b/gcc/c-family/c-lex.c
@@ -258,7 +258,7 @@  cb_def_pragma (cpp_reader *pfile, location_t loc)
 	    name = cpp_token_as_text (pfile, s);
 	}
 
-      warning_at (fe_loc, OPT_Wunknown_pragmas, "ignoring #pragma %s %s",
+      warning_at (fe_loc, OPT_Wunknown_pragmas, "ignoring %<#pragma %s %s%>",
 		  space, name);
     }
 }
@@ -818,7 +818,7 @@  interpret_float (const cpp_token *token, unsigned int flags,
       if (((flags & CPP_N_HEX) == 0) && ((flags & CPP_N_IMAGINARY) == 0))
 	{
 	  warning (OPT_Wunsuffixed_float_constants,
-		   "unsuffixed float constant");
+		   "unsuffixed floating constant");
 	  if (float_const_decimal64_p ())
 	    flags |= CPP_N_DFLOAT;
 	}
diff --git a/gcc/c-family/c-omp.c b/gcc/c-family/c-omp.c
index 16e71981887..5645e9d4fda 100644
--- a/gcc/c-family/c-omp.c
+++ b/gcc/c-family/c-omp.c
@@ -974,7 +974,7 @@  c_finish_omp_for (location_t locus, enum tree_code code, tree declv,
 				{
 				  error_at (elocus,
 					    "increment is not constant 1 or "
-					    "-1 for != condition");
+					    "-1 for %<!=%> condition");
 				  fail = true;
 				}
 			    }
@@ -992,7 +992,7 @@  c_finish_omp_for (location_t locus, enum tree_code code, tree declv,
 			{
 			  error_at (elocus,
 				    "increment is not constant 1 or -1 for"
-				    " != condition");
+				    " %<!=%> condition");
 			  fail = true;
 			}
 		    }
diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index bc7ea176909..954b6a494f8 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -1022,8 +1022,8 @@  c_common_post_options (const char **pfilename)
     warn_return_type = 1;
 
   if (num_in_fnames > 1)
-    error ("too many filenames given.  Type %s --help for usage",
-	   progname);
+    error ("too many filenames given; type %<%s %s%> for usage",
+	   progname, "--help");
 
   if (flag_preprocess_only)
     {
@@ -1057,7 +1057,7 @@  c_common_post_options (const char **pfilename)
 	     debug formats we warn here and refuse to load any PCH files.  */
 	  if (write_symbols != NO_DEBUG && write_symbols != DWARF2_DEBUG)
 	    warning (OPT_Wdeprecated,
-		     "the \"%s\" debug format cannot be used with "
+		     "the %qs debug format cannot be used with "
 		     "pre-compiled headers", debug_type_names[write_symbols]);
 	}
       else if (write_symbols != NO_DEBUG && write_symbols != DWARF2_DEBUG)
diff --git a/gcc/c-family/c-pch.c b/gcc/c-family/c-pch.c
index 316fb84f429..2fa7a52ae1e 100644
--- a/gcc/c-family/c-pch.c
+++ b/gcc/c-family/c-pch.c
@@ -406,9 +406,9 @@  c_common_pch_pragma (cpp_reader *pfile, const char *name)
 
   if (!cpp_get_options (pfile)->preprocessed)
     {
-      error ("pch_preprocess pragma should only be used "
+      error ("%<pch_preprocess%> pragma should only be used "
 	     "with %<-fpreprocessed%>");
-      inform (input_location, "use #include instead");
+      inform (input_location, "use %<#include%> instead");
       return;
     }
 
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 6b8ada59460..480d98efd1e 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -91,7 +91,8 @@  pop_alignment (tree id)
   align_stack * entry;
 
   if (alignment_stack == NULL)
-    GCC_BAD ("#pragma pack (pop) encountered without matching #pragma pack (push)");
+    GCC_BAD ("%<#pragma pack (pop)%> encountered without matching "
+	     "%<#pragma pack (push)%>");
 
   /* If we got an identifier, strip away everything above the target
      entry so that the next step will restore the state just below it.  */
@@ -104,8 +105,9 @@  pop_alignment (tree id)
 	    break;
 	  }
       if (entry == NULL)
-	warning (OPT_Wpragmas, "\
-#pragma pack(pop, %E) encountered without matching #pragma pack(push, %E)"
+	warning (OPT_Wpragmas,
+		 "%<#pragma pack(pop, %E)%> encountered without matching "
+		 "%<#pragma pack(push, %E)%>"
 		 , id, id);
     }
 
@@ -197,7 +199,7 @@  handle_pragma_pack (cpp_reader * ARG_UNUSED (dummy))
     warning (OPT_Wpragmas, "junk at end of %<#pragma pack%>");
 
   if (flag_pack_struct)
-    GCC_BAD ("#pragma pack has no effect with %<-fpack-struct%> - ignored");
+    GCC_BAD ("%<#pragma pack%> has no effect with %<-fpack-struct%> - ignored");
 
   if (action != pop)
     switch (align)
@@ -257,7 +259,7 @@  apply_pragma_weak (tree decl, tree value)
       && !DECL_WEAK (decl) /* Don't complain about a redundant #pragma.  */
       && DECL_ASSEMBLER_NAME_SET_P (decl)
       && TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
-    warning (OPT_Wpragmas, "applying #pragma weak %q+D after first use "
+    warning (OPT_Wpragmas, "applying %<#pragma weak %+D%> after first use "
 	     "results in unspecified behavior", decl);
 
   declare_weak (decl);
@@ -354,12 +356,12 @@  handle_pragma_weak (cpp_reader * ARG_UNUSED (dummy))
   value = 0;
 
   if (pragma_lex (&name) != CPP_NAME)
-    GCC_BAD ("malformed #pragma weak, ignored");
+    GCC_BAD ("malformed %<#pragma weak%>, ignored");
   t = pragma_lex (&x);
   if (t == CPP_EQ)
     {
       if (pragma_lex (&value) != CPP_NAME)
-	GCC_BAD ("malformed #pragma weak, ignored");
+	GCC_BAD ("%<malformed #pragma weak%>, ignored");
       t = pragma_lex (&x);
     }
   if (t != CPP_EOF)
@@ -417,7 +419,7 @@  handle_pragma_scalar_storage_order (cpp_reader *ARG_UNUSED(dummy))
 
   if (BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN)
     {
-      error ("scalar_storage_order is not supported because endianness "
+      error ("%<scalar_storage_order%> is not supported because endianness "
 	     "is not uniform");
       return;
     }
@@ -495,9 +497,9 @@  handle_pragma_redefine_extname (cpp_reader * ARG_UNUSED (dummy))
   bool found;
 
   if (pragma_lex (&oldname) != CPP_NAME)
-    GCC_BAD ("malformed #pragma redefine_extname, ignored");
+    GCC_BAD ("malformed %<#pragma redefine_extname%>, ignored");
   if (pragma_lex (&newname) != CPP_NAME)
-    GCC_BAD ("malformed #pragma redefine_extname, ignored");
+    GCC_BAD ("malformed %<#pragma redefine_extname%>, ignored");
   t = pragma_lex (&x);
   if (t != CPP_EOF)
     warning (OPT_Wpragmas, "junk at end of %<#pragma redefine_extname%>");
@@ -528,8 +530,8 @@  handle_pragma_redefine_extname (cpp_reader * ARG_UNUSED (dummy))
 	      name = targetm.strip_name_encoding (name);
 
 	      if (!id_equal (newname, name))
-		warning (OPT_Wpragmas, "#pragma redefine_extname ignored due to "
-			 "conflict with previous rename");
+		warning (OPT_Wpragmas, "%<#pragma redefine_extname%> "
+			 "ignored due to conflict with previous rename");
 	    }
 	  else
 	    symtab->change_decl_assembler_name (decl, newname);
@@ -556,8 +558,8 @@  add_to_renaming_pragma_list (tree oldname, tree newname)
     if (oldname == p->oldname)
       {
 	if (p->newname != newname)
-	  warning (OPT_Wpragmas, "#pragma redefine_extname ignored due to "
-		   "conflict with previous #pragma redefine_extname");
+	  warning (OPT_Wpragmas, "%<#pragma redefine_extname%> ignored due to "
+		   "conflict with previous %<#pragma redefine_extname%>");
 	return;
       }
 
@@ -592,7 +594,7 @@  maybe_apply_renaming_pragma (tree decl, tree asmname)
       oldname = targetm.strip_name_encoding (oldname);
 
       if (asmname && strcmp (TREE_STRING_POINTER (asmname), oldname))
-	  warning (OPT_Wpragmas, "asm declaration ignored due to "
+	  warning (OPT_Wpragmas, "%<asm%> declaration ignored due to "
 		   "conflict with previous rename");
 
       /* Take any pending redefine_extname off the list.  */
@@ -601,8 +603,8 @@  maybe_apply_renaming_pragma (tree decl, tree asmname)
 	  {
 	    /* Only warn if there is a conflict.  */
 	    if (!id_equal (p->newname, oldname))
-	      warning (OPT_Wpragmas, "#pragma redefine_extname ignored due to "
-		       "conflict with previous rename");
+	      warning (OPT_Wpragmas, "%<#pragma redefine_extname%> ignored "
+		       "due to conflict with previous rename");
 
 	    pending_redefine_extname->unordered_remove (ix);
 	    break;
@@ -623,8 +625,8 @@  maybe_apply_renaming_pragma (tree decl, tree asmname)
 	  {
 	    if (strcmp (TREE_STRING_POINTER (asmname),
 			IDENTIFIER_POINTER (newname)) != 0)
-	      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");
 	    return asmname;
 	  }
 
@@ -684,7 +686,8 @@  push_visibility (const char *str, int kind)
   else if (!strcmp (str, "protected"))
     default_visibility = VISIBILITY_PROTECTED;
   else
-    GCC_BAD ("#pragma GCC visibility push() must specify default, internal, hidden or protected");
+    GCC_BAD ("%<#pragma GCC visibility push()%> must specify %<default%>, "
+	     "%<internal%>, %<hidden%> or %<protected%>");
   visibility_options.inpragma = 1;
 }
 
@@ -726,7 +729,8 @@  handle_pragma_visibility (cpp_reader *dummy ATTRIBUTE_UNUSED)
 	action = pop;
     }
   if (bad == action)
-    GCC_BAD ("#pragma GCC visibility must be followed by push or pop");
+    GCC_BAD ("%<#pragma GCC visibility%> must be followed by %<push%> "
+	     "or %<pop%>");
   else
     {
       if (pop == action)
@@ -740,7 +744,7 @@  handle_pragma_visibility (cpp_reader *dummy ATTRIBUTE_UNUSED)
 	    GCC_BAD ("missing %<(%> after %<#pragma GCC visibility push%> - ignored");
 	  token = pragma_lex (&x);
 	  if (token != CPP_NAME)
-	    GCC_BAD ("malformed #pragma GCC visibility push");
+	    GCC_BAD ("malformed %<#pragma GCC visibility push%>");
 	  else
 	    push_visibility (IDENTIFIER_POINTER (x), 0);
 	  if (pragma_lex (&x) != CPP_CLOSE_PAREN)
@@ -860,7 +864,7 @@  handle_pragma_target(cpp_reader *ARG_UNUSED(dummy))
 
   if (cfun)
     {
-      error ("#pragma GCC option is not allowed inside functions");
+      error ("%<#pragma GCC option%> is not allowed inside functions");
       return;
     }
 
@@ -906,7 +910,7 @@  handle_pragma_target(cpp_reader *ARG_UNUSED(dummy))
 
       if (token != CPP_EOF)
 	{
-	  error ("#pragma GCC target string... is badly formed");
+	  error ("%<#pragma GCC target%> string is badly formed");
 	  return;
 	}
 
@@ -929,7 +933,7 @@  handle_pragma_optimize (cpp_reader *ARG_UNUSED(dummy))
 
   if (cfun)
     {
-      error ("#pragma GCC optimize is not allowed inside functions");
+      error ("%<#pragma GCC optimize%> is not allowed inside functions");
       return;
     }
 
@@ -974,7 +978,7 @@  handle_pragma_optimize (cpp_reader *ARG_UNUSED(dummy))
 
       if (token != CPP_EOF)
 	{
-	  error ("#pragma GCC optimize string... is badly formed");
+	  error ("%<#pragma GCC optimize%> string is badly formed");
 	  return;
 	}
 
@@ -1147,7 +1151,8 @@  handle_pragma_message (cpp_reader *ARG_UNUSED(dummy))
     warning (OPT_Wpragmas, "junk at end of %<#pragma message%>");
 
   if (TREE_STRING_LENGTH (message) > 1)
-    inform (input_location, "#pragma message: %s", TREE_STRING_POINTER (message));
+    inform (input_location, "%<#pragma message: %s%>",
+	    TREE_STRING_POINTER (message));
 }
 
 /* Mark whether the current location is valid for a STDC pragma.  */
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index f95eba96a70..fa7b6ddea55 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -1663,7 +1663,7 @@  warn_for_omitted_condop (location_t location, tree cond)
       || (TREE_TYPE (cond) != NULL_TREE
 	  && TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE))
       warning_at (location, OPT_Wparentheses,
-		"the omitted middle operand in ?: will always be %<true%>, "
+		"the omitted middle operand in %<?:%> will always be %<true%>, "
 		"suggest explicit middle operand");
 }
 
@@ -1762,7 +1762,7 @@  lvalue_error (location_t loc, enum lvalue_use use)
       error_at (loc, "lvalue required as unary %<&%> operand");
       break;
     case lv_asm:
-      error_at (loc, "lvalue required in asm statement");
+      error_at (loc, "lvalue required in %<asm%> statement");
       break;
     default:
       gcc_unreachable ();
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 916cc67b453..046d489f7eb 100644