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