diff mbox

Add fixit hint for -Wlogical-not-parentheses

Message ID 20160825081623.GN11131@redhat.com
State New
Headers show

Commit Message

Marek Polacek Aug. 25, 2016, 8:16 a.m. UTC 
On Wed, Aug 24, 2016 at 04:03:10PM -0400, David Malcolm wrote:
> 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.
 
Yes, sure.

> 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?

Ok, how about this one?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-08-25  Marek Polacek  <polacek@redhat.com>
	    David Malcolm  <dmalcolm@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.


	Marek

Comments

David Malcolm Aug. 25, 2016, 12:40 p.m. UTC | #1 
On Thu, 2016-08-25 at 10:16 +0200, Marek Polacek wrote:
> On Wed, Aug 24, 2016 at 04:03:10PM -0400, David Malcolm wrote:
> > 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.
>  
> Yes, sure.
> 
> > 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?
> 
> Ok, how about this one?

Looks good to me, based on the above discussion.  I'll try to add the
shared fixit validation code today.

> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2016-08-25  Marek Polacek  <polacek@redhat.com>
> 	    David Malcolm  <dmalcolm@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..001070d 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,21 @@ 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, "(");
> +      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");
> +    }
>  }
>  
>  /* 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..ba8dce8 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;
> +}
> 
> 	Marek
Marek Polacek Aug. 25, 2016, 12:43 p.m. UTC | #2 
On Thu, Aug 25, 2016 at 08:40:20AM -0400, David Malcolm wrote:
> Looks good to me, based on the above discussion.  I'll try to add the
> shared fixit validation code today.

Thanks.  Let me know if I can be of any help with that.

	Marek
Andreas Schwab Aug. 29, 2016, 9:16 a.m. UTC | #3 
On Aug 25 2016, Marek Polacek <polacek@redhat.com> wrote:

> 	* c-c++-common/Wlogical-not-parentheses-2.c: New test.

FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11  expected multiline pattern lines 13-17 not found: "\s*r \+= !aaa == bbb;.*\n             \^~\n   r \+= !aaa == bbb;.*\n        \^~~~\n        \(   \).*\n"
FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11 (test for excess errors)
Excess errors:
   r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
             ^~
   r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
        ^~~~

Andreas.
Marek Polacek Aug. 29, 2016, 10:35 a.m. UTC | #4 
On Mon, Aug 29, 2016 at 11:16:25AM +0200, Andreas Schwab wrote:
> On Aug 25 2016, Marek Polacek <polacek@redhat.com> wrote:
> 
> > 	* c-c++-common/Wlogical-not-parentheses-2.c: New test.
> 
> FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11  expected multiline pattern lines 13-17 not found: "\s*r \+= !aaa == bbb;.*\n             \^~\n   r \+= !aaa == bbb;.*\n        \^~~~\n        \(   \).*\n"
> FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11 (test for excess errors)
> Excess errors:
>    r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
>              ^~
>    r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
>         ^~~~

This has regressed with David's

commit 367964faf71a99ebd511dffb81075b58bff345a1
Author: dmalcolm <dmalcolm@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Aug 26 21:25:41 2016 +0000

    Add validation and consolidation of fix-it hints
    
I don't know yet what exactly went wrong here.

	Marek
Marek Polacek Aug. 29, 2016, 11:44 a.m. UTC | #5 
On Mon, Aug 29, 2016 at 12:35:38PM +0200, Marek Polacek wrote:
> On Mon, Aug 29, 2016 at 11:16:25AM +0200, Andreas Schwab wrote:
> > On Aug 25 2016, Marek Polacek <polacek@redhat.com> wrote:
> > 
> > > 	* c-c++-common/Wlogical-not-parentheses-2.c: New test.
> > 
> > FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11  expected multiline pattern lines 13-17 not found: "\s*r \+= !aaa == bbb;.*\n             \^~\n   r \+= !aaa == bbb;.*\n        \^~~~\n        \(   \).*\n"
> > FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11 (test for excess errors)
> > Excess errors:
> >    r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
> >              ^~
> >    r += !aaa == bbb; /* { dg-warning "logical not is only applied" } */
> >         ^~~~
> 
> This has regressed with David's
> 
> commit 367964faf71a99ebd511dffb81075b58bff345a1
> Author: dmalcolm <dmalcolm@138bc75d-0d04-0410-961f-82ee72b054a4>
> Date:   Fri Aug 26 21:25:41 2016 +0000
> 
>     Add validation and consolidation of fix-it hints
>     
> I don't know yet what exactly went wrong here.

So we reject printing fix-it hint because in reject_impossible_fixit:

2187   if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
2188     /* WHERE is a reasonable location for a fix-it; don't reject it.  */
2189     return false;

(gdb) p where
$1 = 2147483652
(gdb) p LINE_MAP_MAX_LOCATION_WITH_COLS
$2 = 1610612736

so we set m_seen_impossible_fixit.

David, why is that happening?

	Marek
David Malcolm Aug. 29, 2016, 12:50 p.m. UTC | #6 
On Mon, 2016-08-29 at 13:44 +0200, Marek Polacek wrote:
> On Mon, Aug 29, 2016 at 12:35:38PM +0200, Marek Polacek wrote:
> > On Mon, Aug 29, 2016 at 11:16:25AM +0200, Andreas Schwab wrote:
> > > On Aug 25 2016, Marek Polacek <polacek@redhat.com> wrote:
> > > 
> > > > 	* c-c++-common/Wlogical-not-parentheses-2.c: New test.
> > > 
> > > FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11 
> > >  expected multiline pattern lines 13-17 not found: "\s*r \+= !aaa
> > > == bbb;.*\n             \^~\n   r \+= !aaa == bbb;.*\n       
> > >  \^~~~\n        \(   \).*\n"
> > > FAIL: c-c++-common/Wlogical-not-parentheses-2.c  -std=gnu++11
> > > (test for excess errors)
> > > Excess errors:
> > >    r += !aaa == bbb; /* { dg-warning "logical not is only
> > > applied" } */
> > >              ^~
> > >    r += !aaa == bbb; /* { dg-warning "logical not is only
> > > applied" } */
> > >         ^~~~
> > 
> > This has regressed with David's
> > 
> > commit 367964faf71a99ebd511dffb81075b58bff345a1
> > Author: dmalcolm <dmalcolm@138bc75d-0d04-0410-961f-82ee72b054a4>
> > Date:   Fri Aug 26 21:25:41 2016 +0000
> > 
> >     Add validation and consolidation of fix-it hints
> >     
> > I don't know yet what exactly went wrong here.
> 
> So we reject printing fix-it hint because in reject_impossible_fixit:
> 
> 2187   if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
> 2188     /* WHERE is a reasonable location for a fix-it; don't reject
> it.  */
> 2189     return false;
> 
> (gdb) p where
> $1 = 2147483652
> (gdb) p LINE_MAP_MAX_LOCATION_WITH_COLS
> $2 = 1610612736
> 
> so we set m_seen_impossible_fixit.
> 
> David, why is that happening?

Sorry about this; I believe I did my testing of my patch against a tree
that didn't yet have your change.

(gdb) p /x 2147483652
$2 = 0x80000004

so "where", the insertion-point, is an ad-hoc location (describing a
range), and it looks like I somehow forgot to deal with that case in
the validation code.

I'm working on a fix.  Sorry again.

Dave
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 3feb910..001070d 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,21 @@  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, "(");
+      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");
+    }
 }
 
 /* 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..ba8dce8 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;
+}