On Wed, Aug 24, 2016 at 02:59:43PM -0400, David Malcolm wrote:
> On Wed, 2016-08-24 at 20:15 +0200, Marek Polacek wrote:
> > On Wed, Aug 24, 2016 at 01:57:15PM -0400, David Malcolm wrote:
> > > On Wed, 2016-08-24 at 19:43 +0200, Marek Polacek wrote:
> > > > This patch adds a fixit hint to -Wlogical-not-parentheses. When
> > > > the
> > > > LHS
> > > > has a location, it prints:
> > > >
> > > > z.c: In function ‘int foo(int, int)’:
> > > > z.c:12:11: warning: logical not is only applied to the left hand
> > > > side
> > > > of comparison [-Wlogical-not-parentheses]
> > > > r += !a == b;
> > > > ^~
> > > > z.c:12:8: note: add parentheses around left hand side expression
> > > > to
> > > > silence this warning
> > > > r += !a == b;
> > > > ^~
> > > > ( )
> > > >
> > > > Bootstrapped/regtested on x86_64-linux and ppc64le-redhat-linux,
> > > > ok
> > > > for trunk?
> > >
> > > Thanks for writing new fix-it hints.
> > >
> > > Can you split this up into two fix-its, one for each parenthesis,
> > > at
> > > the appropriate locations? A single rich_location (and thus
> > > diagnostic) can contain up to 3 fix-it hints at the moment. My
> > > hope
> > > is that an IDE could, in theory, apply them; right now, the fixit
> > > is
> > > effectively saying to make this change:
> > >
> > > - r += !a == b;
> > > + r += ( )!a == b;
> > >
> > > whereas presumably it should be making this change:
> > >
> > > - r += !a == b;
> > > + r += (!a) == b;
> >
> > True.
> >
> > > You should be able to access the source end-point of the expression
> > > via:
> > > get_range_from_loc (line_table, loc).m_finish
> >
> > I see, thanks. So like in the below?
>
> Thanks. I didn't fully think through my suggestion, sorry. I think
> there's an off-by-one error in the positioning of the closing
> parenthesis, though up till now we haven't defined the semantics of
> fixits very strongly.
>
> My interpretation is that an insertion fixit happens immediately
> *before* the current content of its column.
>
> So given these column numbers:
> 0000000001111111111
> 1234567890123456789
> r += !aaa == bbb;
>
> we want to:
> (i) insert a '(' immediately before the '! i.e. at column 8 (correct in
> the patch), and
> (ii) insert a ')' after the "aaa" i.e. immediately before the ' ' after
> the "aaa" i.e. at column 12
>
> I tried your patch with my "-fdiagnostics-generate-patch" patch, and
> the patch it generated for these fixits was:
> --- ../../src/gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> +++ ../../src/gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> @@ -8,7 +8,7 @@
> {
> int r = 0;
> r += (!aaa) == bbb;
> - r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
> + r += (!aa)a == bbb; /* { dg-warning "logical not is only applied" } */
> /* { dg-begin-multiline-output "" }
> r += !aaa == bbb;
> ^~
>
> Note the incorrect:
> (!aa)a
> rather than:
> (!aaa)
>
> which is consistent with the interpretation above.
>
> So I think you need to offset that final column by 1, which isn't as
> simple as simply adding 1 to the location_t (given packed ranges).
>
> I believe you need something like:
>
> next_loc = linemap_position_for_loc_and_offset (line_table, finish, 1)
>
> I've attached a patch to your patch that does this; with that, the
> fixits and
> generated patch look like this:
Makes sense, thanks. Before I post another patch, should I introduce
a helper for this, something like get_one_past_finish or somesuch?
Marek
