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
