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

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

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

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.

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

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

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

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

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

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

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.

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