This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: 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:
--- ../../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:


../../src/gcc/testsuite/c-c++-common/Wlogical-not-parentheses-2.c:11:8: note:
add parentheses around left hand side expression to silence this warning
   r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
        ^~~~
        (   )
--- ../../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 += (!aaa) == bbb; /* { dg-warning "logical not is only applied" }
*/
 /* { dg-begin-multiline-output "" }
    r += !aaa == bbb;
              ^~

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
From 77a245f07749c2b175e3f15186fe1fe995b2a33d Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Wed, 24 Aug 2016 15:19:49 -0400
Subject: [PATCH] Fix off-by-one column issue in fixit

---
 gcc/c-family/c-common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1059466..001070d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -1506,7 +1506,10 @@ warn_logical_not_parentheses (location_t location, enum tree_code code,
       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), ")");
+      location_t finish = get_finish (lhs_loc);
+      location_t next_loc
+	= linemap_position_for_loc_and_offset (line_table, finish, 1);
+      richloc.add_fixit_insert (next_loc, ")");
       inform_at_rich_loc (&richloc, "add parentheses around left hand side "
 			  "expression to silence this warning");
     }
-- 
1.8.5.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]