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
