On 07/11/2017 12:32 PM, David Malcolm wrote:
On Tue, 2017-07-11 at 11:28 -0600, Martin Sebor wrote:
On 07/11/2017 09:24 AM, David Malcolm wrote:
[This patch kit is effectively just one patch; I've split it up
into
3 parts, in the hope of making it easier to review:
the c-family parts, the C parts, and the C++ parts]
This patch adds a hint to the user to various errors generated
in the C frontend by:
c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>")
c_parser_skip_until_found (parser, CPP_CLOSE_BRACE, "expected
%<}%>")
etc (where there's a non-NULL msgid), and in the C++ frontend by:
cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN)
cp_parser_require (parser, CPP_CLOSE_BRACE, RT_CLOSE_BRACE)
The hint shows the user where the pertinent open paren or open
brace
is, which ought to be very helpful for complicated nested
collections
of parentheses, and somewhat helpful even for simple cases;
I've played with the patch a bit. First, let me say that I like
how it associates the curlies. I agree that it will be helpful.
There are other cases where highlighting mismatched or missing
tokens might be useful, such as for pairs of < and > in complex
template declarations.
Indeed; braces and parens seemed most useful; my plan is to leave
< > and [ ] for followups.
But I mainly experimented with it to see if I could get it to
manifest some of the same symptoms I described in bug 81269.
I'm not sure it does reproduce the exact same thing or if it's
a feature, so let me use this as an opportunity to ask. Given
something like
namespace {
enum { e
<EOF>
I see this output:
a.C:3:8: error: expected ‘}’ at end of input
enum { e
~ ^
a.C:3:8: error: expected unqualified-id at end of input
a.C:3:8: error: expected ‘}’ at end of input
a.C:1:11: note: to match this ‘{’
namespace {
^
I think this is an issue with how the diagnostics subsystem chooses
colors; it looks like it's time to rethink that.
Here's what's currently going on:
The color-selection is done in class colorizer in diagnostic-show
-locus.c; the exact colors are in diagnostic-colors.c
with the first open curly/caret in green,
This is range 1 within its rich_location, and so colorizer uses state
1, and hence uses the color named "range1", which is implemented as
COLOR_FG_GREEN aka "range1=32" in GCC_COLORS.
the 'e' in red, and
the last open curly/caret in cyan.
This is range 0 within its rich_location, and so colorizer uses the
state 0:
/* Make range 0 be the same color as the "kind" text
(error vs warning vs note). */
This diagnostic is an error, and hence it uses the color named "error",
which is bold red.
I see. Thanks for the detailed explanation! It cleared things
up for me.
Is the green color intended? And if yes, what is the intent of
distinguishing it from the red 'e'? I note that the caret is
red (and there are no other colors in the output) in this case:
namespace { enum {
<EOF>
but becomes green again when I add an enumerator:
namespace { enum { e
<EOF>
Is there an extra line here that you trimmed? Presumably in this case
the '{' is being underlined with a "~", and the "e" has the "^" (the
caret)?
The source file has just one line in both instances, but yes,
the underlining is as you described.
In this case, the red is used for that of the caret, and the green for
the secondary location.
Aha! I think I understand now. In the first case there is no
green because both the primary and the secondary location are
the unmatched open curly. That makes sense, though it's not
quite obvious (I haven't worked with secondary locations yet
or noticed them in other diagnostics).
If you don't completely rework things (and even if you do) it
would be helpful to document this in more detail in the manual.
E.g., describe what range1 and range2 (and other less obvious
elements) are used for and show a couple of examples. Or for
GCC developers, add some of this detail to one of the Wiki
pages.
I picked this system for coloring the source and annotations in gcc 6
(IIRC), but it seems garish to me now; I think I now favor emulating
what clang does, which is to *not* color the printed source lines, and
to use just one color (green) for underlines and carets.
I'm sure it's a matter of personal preference but overall I like
the GCC highlighting scheme better. It makes it clear what the
source of the problem is. Especially with fix-it hints in green,
it makes it clear what's "good" and what's "bad."
Can I use PR 81269 for tracking a refresh of how we do colorization in
diagnostics?
Sure, feel free to use it however you see fit.
I ask because in the test case in 81269, highlighting the different
tokens in the three colors seems especially confusing and I'd like
to better understand if it's intentional (and what it means).
Incidentally, I tried to make use of this feature in the middle
end (in gimple-ssa-sprintf.c), to achieve the same effect as
-Wrestric does, but it led to even stranger-looking results so
I went back to using plain old warning. See the attachment to
bug 81269:
https://gcc.gnu.org/bugzilla/attachment.cgi?id=41660
Looks like you're seeing a different bug there: you're seeing:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
with weird-looking colorization of some arguments.
I think what's going on is we have:
primary location:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
^
secondary location 1:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
to cover: ^
but this PARAM_DECL usage doesn't have a location, and so
it uses the whole of the call
secondary location 1:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
~~^~~
The bug here I think is that diagnostic_show_locus is printing all of these
annotation on top of each other, rather than trying to add new lines:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
^ (for loc 0)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (for loc 1)
~~^~~ (for loc 2)
or:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
^ ~~^~~ (for locs 0 and 2)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (for loc 1)
Although the ideal here would be for the usages of "d" to have their own
locations, giving:
sprintf (d, "%s%s%s", d, d + 1, d + 2);
^ ^ ~~^~~
That could be. The effect I was hoping for was close to this
https://gcc.gnu.org/bugzilla/attachment.cgi?id=41659
except either with all arguments in the same color, or with
the first in the primary color and the rest in the secondary
color. I don't quite understand where the cyan comes from.
Maybe the caller (warn_for_restrict in c-family/c-warn.c)
needs to do some more work to make it look like that?
(Not sure if it's worth implementing the extra smarts within
diagnostic_show_locus vs trying to fix things so that uses of PARM_DELCs have
their own source locations).
The latter please :)
Despite some of these minor glitches what GCC does today is
fantastic -- thank you!
Martin
