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
