Add fixit hint for -Wlogical-not-parentheses

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:
              ^~

which looks correct to me.


> > Also, when writing testcases that involve -fdiagnostics-show-caret,
> > please use identifier names that are longer than one character:
> > this
> > makes it easier to verify that we're using the correct ranges.
>  
> Done.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2016-08-24  Marek Polacek  <polacek@redhat.com>
> 
> 	* c-common.c (warn_logical_not_parentheses): Print fixit hints.
> 	* c-common.h (warn_logical_not_parentheses): Update
> declaration.
> 
> 	* c-typeck.c (parser_build_binary_op): Pass LHS to
> 	warn_logical_not_parentheses.
> 
> 	* parser.c (cp_parser_binary_expression): Pass LHS to
> 	warn_logical_not_parentheses.
> 
> 	* c-c++-common/Wlogical-not-parentheses-2.c: New test.
> 
> diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
> index 3feb910..1059466 100644
> --- gcc/c-family/c-common.c
> +++ gcc/c-family/c-common.c
> @@ -1485,7 +1485,7 @@ warn_tautological_cmp (location_t loc, enum
> tree_code code, tree lhs, tree rhs)
>  
>  void
>  warn_logical_not_parentheses (location_t location, enum tree_code
> code,
> -			      tree rhs)
> +			      tree lhs, tree rhs)
>  {
>    if (TREE_CODE_CLASS (code) != tcc_comparison
>        || TREE_TYPE (rhs) == NULL_TREE
> @@ -1498,9 +1498,18 @@ warn_logical_not_parentheses (location_t
> location, enum tree_code code,
>        && integer_zerop (rhs))
>      return;
>  
> -  warning_at (location, OPT_Wlogical_not_parentheses,
> -	      "logical not is only applied to the left hand side of
> "
> -	      "comparison");
> +  if (warning_at (location, OPT_Wlogical_not_parentheses,
> +		  "logical not is only applied to the left hand side
> of "
> +		  "comparison")
> +      && EXPR_HAS_LOCATION (lhs))
> +    {
> +      location_t lhs_loc = EXPR_LOCATION (lhs);
> +      rich_location richloc (line_table, lhs_loc);
> +      richloc.add_fixit_insert (lhs_loc, "(");
> +      richloc.add_fixit_insert (get_finish (lhs_loc), ")");
> +      inform_at_rich_loc (&richloc, "add parentheses around left
> hand side "
> +			  "expression to silence this warning");
> +    }
>  }
>  
>  /* Warn if EXP contains any computations whose results are not used.
> diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
> index bc22baa..42ce969 100644
> --- gcc/c-family/c-common.h
> +++ gcc/c-family/c-common.h
> @@ -847,7 +847,8 @@ extern void overflow_warning (location_t, tree);
>  extern bool warn_if_unused_value (const_tree, location_t);
>  extern void warn_logical_operator (location_t, enum tree_code, tree,
>  				   enum tree_code, tree, enum
> tree_code, tree);
> -extern void warn_logical_not_parentheses (location_t, enum
> tree_code, tree);
> +extern void warn_logical_not_parentheses (location_t, enum
> tree_code, tree,
> +					  tree);
>  extern void warn_tautological_cmp (location_t, enum tree_code, tree,
> tree);
>  extern void check_main_parameter_types (tree decl);
>  extern bool c_determine_visibility (tree);
> diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
> index bc8728a..2f8d611 100644
> --- gcc/c/c-typeck.c
> +++ gcc/c/c-typeck.c
> @@ -3696,7 +3696,7 @@ parser_build_binary_op (location_t location,
> enum tree_code code,
>  	  while (1);
>  	}
>        if (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE)
> -	warn_logical_not_parentheses (location, code, arg2.value);
> +	warn_logical_not_parentheses (location, code, arg1.value,
> arg2.value);
>      }
>  
>    /* Warn about comparisons against string literals, with the
> exception
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index 690e928..d54cf8a 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -8922,7 +8922,7 @@ cp_parser_binary_expression (cp_parser* parser,
> bool cast_p,
>  	      || TREE_TYPE (current.lhs) == NULL_TREE
>  	      || TREE_CODE (TREE_TYPE (current.lhs)) !=
> BOOLEAN_TYPE))
>  	warn_logical_not_parentheses (current.loc,
> current.tree_type,
> -				      maybe_constant_value (rhs));
> +				      current.lhs,
> maybe_constant_value (rhs));
>  
>        overload = NULL;
>  
> diff --git gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> index e69de29..c70adc5 100644
> --- gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> +++ gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wlogical-not-parentheses -fdiagnostics-show-caret"
> } */
> +
> + /* Test fixit hints.  */
> +
> +int
> +foo (int aaa, int bbb)
> +{
> +  int r = 0;
> +  r += (!aaa) == bbb;
> +  r += !aaa == bbb; /* { dg-warning "logical not is only applied" }
> */
> +/* { dg-begin-multiline-output "" }
> +   r += !aaa == bbb;
> +             ^~
> +   r += !aaa == bbb;
> +        ^~~~
> +        (  )
> +   { dg-end-multiline-output "" } */
> +  return r;
> +}
> 
> 	Mar

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

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