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
