diff mbox

Add fixit hint for -Wlogical-not-parentheses

Message ID 20160824181549.GL11131@redhat.com
State New
Headers show

Commit Message

Marek Polacek Aug. 24, 2016, 6:15 p.m. UTC 
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?

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


	Marek
diff mbox

Patch

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;
+}