Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2016-08-24 at 21:50 +0200, Marek Polacek wrote:
> 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?

I was thinking about maybe:
  rich_location::add_fixit_insert_after_caret (source_location loc)
which would add it immediately after the *caret* location of loc - as
opposed to after the *finish* location of loc, or maybe just:
  rich_location::add_fixit_insert_after (source_location loc)
which
would take into account the finish location of loc.

Having a helper in rich_location for this would be good, for protecting against cases where some of the tokens are in a macro expansion, and others aren't (I think fix-its within a rich_location ought to be atomic: if some can't be applied, none should be).  I'd rather have the rich_location machinery care about these awkward issues in one place, rather than force every diagnostic to do it.

I already have the beginnings of some fixit validation code written (but not posted), so maybe you should go ahead and post the patch as-is, and we can move the relevant parts into a helper as a followup?

Dave