Add fixit hint for -Wlogical-not-parentheses

Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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?

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..a445df1 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,16 @@ 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))
+    {
+      rich_location richloc (line_table, EXPR_LOCATION (lhs));
+      richloc.add_fixit_insert (EXPR_LOCATION (lhs), "( )");
+      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..c5c2aac 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 a, int b)
+{
+  int r = 0;
+  r += (!a) == b;
+  r += !a == b; /* { dg-warning "logical not is only applied" } */
+/* { dg-begin-multiline-output "" }
+   r += !a == b;
+           ^~
+   r += !a == b;
+        ^~
+        ( )
+   { dg-end-multiline-output "" } */
+  return r;
+}

	Marek

Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm]

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;

You should be able to access the source end-point of the expression
via:
  get_range_from_loc (line_table, loc).m_finish

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.


> 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..a445df1 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,16 @@ 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))
> +    {
> +      rich_location richloc (line_table, EXPR_LOCATION (lhs));
> +      richloc.add_fixit_insert (EXPR_LOCATION (lhs), "( )");
> +      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..c5c2aac 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 a, int b)
> +{
> +  int r = 0;
> +  r += (!a) == b;
> +  r += !a == b; /* { dg-warning "logical not is only applied" } */
> +/* { dg-begin-multiline-output "" }
> +   r += !a == b;
> +           ^~
> +   r += !a == b;
> +        ^~
> +        ( )
> +   { dg-end-multiline-output "" } */
> +  return r;
> +}
> 
> 	Marek

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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.

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

	Marek

Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 8668 bytes --]

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

[-- Attachment #2: 0001-Fix-off-by-one-column-issue-in-fixit.patch --]
[-- Type: text/x-patch, Size: 1085 bytes --]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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.

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

Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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

[committed] Add validation and consolidation of fix-it hints

[David Malcolm]

The first aspect of this patch is to add some checking of fix-it hints.
The idea is to put this checking within the rich_location machinery,
rather than requiring every diagnostic to implement it for itself.

The fixits within a rich_location are "atomic": all must be valid for
any to be applicable.

We reject any fixits involving locations above
LINE_MAP_MAX_LOCATION_WITH_COLS.

There's no guarantee that it's sane to modify a macro, so we reject
any fix-its that touch them.

For example, note the attempt to provide a fix-it for the definition
of the macro FIELD:

spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
 #define FIELD colour
               ^
               color
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
   return ptr->FIELD;
               ^~~~~

After this patch, the fixit is not displayed:

spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
 #define FIELD colour
               ^
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
   return ptr->FIELD;
               ^~~~~

We might want some way for a diagnostic to opt-in to fix-its that
affect macros, but for now it's simplest to reject them.

The other aspect of this patch is fix-it consolidation: in some cases
neighboring fix-its can be merged.  For example, in a diagnostic to
modernize old-style struct initializers from:

 struct s example = {
- foo: 1,
+ .foo = 1,
 };

one approach would be to replace the "foo" with ".foo" and the ":"
with " =".  This would give two "replace" fix-its:

  foo: 1,
  --- FIXIT 1
  .foo
     - FIXIT 2
     =

This patch allows them to be consolidated into a single "replace" fix-it:

  foo: 1,
  ----
  .foo =

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

Committed to trunk as r239789.

gcc/ChangeLog:
	* diagnostic-show-locus.c
	(selftest::test_fixit_consolidation): New function.
	(selftest::diagnostic_show_locus_c_tests): Call it.
	* gcc-rich-location.h (gcc_rich_location): Eliminate unused
	constructor based on source_range.

gcc/testsuite/ChangeLog:
	* gcc.dg/spellcheck-fields-2.c (test): Move
	dg-begin/end-multiline-output within function body.
	(test_macro): New function.

libcpp/ChangeLog:
	* include/line-map.h (rich_location): Eliminate unimplemented
	constructor based on source_range.
	(rich_location::get_last_fixit_hint): New method.
	(rich_location::reject_impossible_fixit): New method.
	(rich_location): Add fields m_line_table and
	m_seen_impossible_fixit.
	(fixit_hint::maybe_append_replace): New pure virtual function.
	(fixit_insert::maybe_append_replace): New function.
	(fixit_replace::maybe_append_replace): New function.
	* line-map.c (rich_location::rich_location): Initialize
	m_line_table and m_seen_impossible_fixit.
	(rich_location::add_fixit_insert): Call
	reject_impossible_fixit and bail out if true.
	(column_before_p): New function.
	(rich_location::add_fixit_replace): Call reject_impossible_fixit
	and bail out if true.  Attempt to consolidate with neighboring
	fixits.
	(rich_location::get_last_fixit_hint): New method.
	(rich_location::reject_impossible_fixit): New method.
	(fixit_insert::maybe_append_replace): New method.
	(fixit_replace::maybe_append_replace): New method.
---
 gcc/diagnostic-show-locus.c                | 155 +++++++++++++++++++++++++++++
 gcc/gcc-rich-location.h                    |   5 -
 gcc/testsuite/gcc.dg/spellcheck-fields-2.c |  23 ++++-
 libcpp/include/line-map.h                  |  19 +++-
 libcpp/line-map.c                          | 136 ++++++++++++++++++++++++-
 5 files changed, 327 insertions(+), 11 deletions(-)

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 94b7349..f3f661e 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1628,6 +1628,160 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
   test_one_liner_fixit_replace_equal_secondary_range ();
 }
 
+/* Verify that fix-it hints are appropriately consolidated.
+
+   If any fix-it hints in a rich_location involve locations beyond
+   LINE_MAP_MAX_LOCATION_WITH_COLS, then we can't reliably apply
+   the fix-it as a whole, so there should be none.
+
+   Otherwise, verify that consecutive "replace" and "remove" fix-its
+   are merged, and that other fix-its remain separate.   */
+
+static void
+test_fixit_consolidation (const line_table_case &case_)
+{
+  line_table_test ltt (case_);
+
+  linemap_add (line_table, LC_ENTER, false, "test.c", 1);
+
+  const location_t c10 = linemap_position_for_column (line_table, 10);
+  const location_t c15 = linemap_position_for_column (line_table, 15);
+  const location_t c16 = linemap_position_for_column (line_table, 16);
+  const location_t c17 = linemap_position_for_column (line_table, 17);
+  const location_t c20 = linemap_position_for_column (line_table, 20);
+  const location_t caret = c10;
+
+  /* Insert + insert. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_insert (c10, "foo");
+    richloc.add_fixit_insert (c15, "bar");
+
+    if (c15 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Insert + replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_insert (c10, "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c15, c17),
+			       "bar");
+
+    if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + non-consecutive insert. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "bar");
+    richloc.add_fixit_insert (c17, "foo");
+
+    if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + non-consecutive replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c17, c20),
+			       "bar");
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + consecutive replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c16, c20),
+			       "bar");
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single "replace".  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("foobar", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+
+  /* Replace + consecutive removal. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single replace, with the
+	   range extended to cover that of the removal.  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("foo", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+
+  /* Consecutive removals. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_remove (source_range::from_locations (c10, c15));
+    richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single "replace-with-empty".  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -1642,6 +1796,7 @@ diagnostic_show_locus_c_tests ()
   test_diagnostic_show_locus_unknown_location ();
 
   for_each_line_table_case (test_diagnostic_show_locus_one_liner);
+  for_each_line_table_case (test_fixit_consolidation);
 }
 
 } // namespace selftest
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index aa69b2e..cc5987f 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -31,11 +31,6 @@ class gcc_rich_location : public rich_location
   gcc_rich_location (source_location loc) :
     rich_location (line_table, loc) {}
 
-  /* Constructing from a source_range.  */
-  gcc_rich_location (source_range src_range) :
-    rich_location (src_range) {}
-
-
   /* Methods for adding ranges via gcc entities.  */
   void
   add_expr (tree expr);
diff --git a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
index d6ebff1..7c54214 100644
--- a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
+++ b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
@@ -9,7 +9,6 @@ union u
 int test (union u *ptr)
 {
   return ptr->colour; /* { dg-error "did you mean .color.?" } */
-}
 
 /* Verify that we get an underline and a fixit hint.  */
 /* { dg-begin-multiline-output "" }
@@ -17,3 +16,25 @@ int test (union u *ptr)
                ^~~~~~
                color
    { dg-end-multiline-output "" } */
+}
+
+
+/* Verify that we don't offer a fixit hint in the presence of
+   a macro.  */
+int test_macro (union u *ptr)
+{
+#define FIELD colour /* { dg-error "did you mean .color.?" } */
+  return ptr->FIELD;
+
+/* { dg-begin-multiline-output "" }
+ #define FIELD colour
+               ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+   return ptr->FIELD;
+               ^~~~~
+   { dg-end-multiline-output "" } */
+
+#undef FIELD
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index a2ed008..0fc4848 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1367,9 +1367,6 @@ class rich_location
   /* Constructing from a location.  */
   rich_location (line_maps *set, source_location loc);
 
-  /* Constructing from a source_range.  */
-  rich_location (source_range src_range);
-
   /* Destructor.  */
   ~rich_location ();
 
@@ -1411,12 +1408,17 @@ class rich_location
 
   unsigned int get_num_fixit_hints () const { return m_num_fixit_hints; }
   fixit_hint *get_fixit_hint (int idx) const { return m_fixit_hints[idx]; }
+  fixit_hint *get_last_fixit_hint () const;
+
+private:
+  bool reject_impossible_fixit (source_location where);
 
 public:
   static const int MAX_RANGES = 3;
   static const int MAX_FIXIT_HINTS = 2;
 
 protected:
+  line_maps *m_line_table;
   unsigned int m_num_ranges;
   location_range m_ranges[MAX_RANGES];
 
@@ -1427,6 +1429,7 @@ protected:
 
   unsigned int m_num_fixit_hints;
   fixit_hint *m_fixit_hints[MAX_FIXIT_HINTS];
+  bool m_seen_impossible_fixit;
 };
 
 class fixit_hint
@@ -1440,6 +1443,10 @@ public:
   virtual bool affects_line_p (const char *file, int line) = 0;
   virtual source_location get_start_loc () const = 0;
   virtual bool maybe_get_end_loc (source_location *out) const = 0;
+  /* Vfunc for consolidating successor fixits.  */
+  virtual bool maybe_append_replace (line_maps *set,
+				     source_range src_range,
+				     const char *new_content) = 0;
 };
 
 class fixit_insert : public fixit_hint
@@ -1452,6 +1459,9 @@ class fixit_insert : public fixit_hint
   bool affects_line_p (const char *file, int line);
   source_location get_start_loc () const { return m_where; }
   bool maybe_get_end_loc (source_location *) const { return false; }
+  bool maybe_append_replace (line_maps *set,
+			     source_range src_range,
+			     const char *new_content);
 
   source_location get_location () const { return m_where; }
   const char *get_string () const { return m_bytes; }
@@ -1478,6 +1488,9 @@ class fixit_replace : public fixit_hint
     *out = m_src_range.m_finish;
     return true;
   }
+  bool maybe_append_replace (line_maps *set,
+			     source_range src_range,
+			     const char *new_content);
 
   source_range get_range () const { return m_src_range; }
   const char *get_string () const { return m_bytes; }
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 3890eff..8fe48bd 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1959,11 +1959,13 @@ source_range::intersects_line_p (const char *file, int line) const
 
 /* Construct a rich_location with location LOC as its initial range.  */
 
-rich_location::rich_location (line_maps */*set*/, source_location loc) :
+rich_location::rich_location (line_maps *set, source_location loc) :
+  m_line_table (set),
   m_num_ranges (0),
   m_column_override (0),
   m_have_expanded_location (false),
-  m_num_fixit_hints (0)
+  m_num_fixit_hints (0),
+  m_seen_impossible_fixit (false)
 {
   add_range (loc, true);
 }
@@ -2075,6 +2077,9 @@ void
 rich_location::add_fixit_insert (source_location where,
 				 const char *new_content)
 {
+  if (reject_impossible_fixit (where))
+    return;
+
   linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
   m_fixit_hints[m_num_fixit_hints++]
     = new fixit_insert (where, new_content);
@@ -2089,6 +2094,44 @@ rich_location::add_fixit_remove (source_range src_range)
   add_fixit_replace (src_range, "");
 }
 
+/* Return true iff A is in the column directly before B, on the
+   same line of the same source file.  */
+
+static bool
+column_before_p (line_maps *set, source_location a, source_location b)
+{
+  if (IS_ADHOC_LOC (a))
+    a = get_location_from_adhoc_loc (set, a);
+  if (IS_ADHOC_LOC (b))
+    b = get_location_from_adhoc_loc (set, b);
+
+  /* They must both be in ordinary maps.  */
+  const struct line_map *linemap_a = linemap_lookup (set, a);
+  if (linemap_macro_expansion_map_p (linemap_a))
+    return false;
+  const struct line_map *linemap_b = linemap_lookup (set, b);
+  if (linemap_macro_expansion_map_p (linemap_b))
+    return false;
+
+  /* To be on the same line, they must be in the same ordinary map.  */
+  if (linemap_a != linemap_b)
+    return false;
+
+  linenum_type line_a
+    = SOURCE_LINE (linemap_check_ordinary (linemap_a), a);
+  linenum_type line_b
+    = SOURCE_LINE (linemap_check_ordinary (linemap_b), b);
+  if (line_a != line_b)
+    return false;
+
+  linenum_type column_a
+    = SOURCE_COLUMN (linemap_check_ordinary (linemap_a), a);
+  linenum_type column_b
+    = SOURCE_COLUMN (linemap_check_ordinary (linemap_b), b);
+
+  return column_b == column_a + 1;
+}
+
 /* Add a fixit-hint, suggesting replacement of the content at
    SRC_RANGE with NEW_CONTENT.  */
 
@@ -2097,10 +2140,67 @@ rich_location::add_fixit_replace (source_range src_range,
 				  const char *new_content)
 {
   linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+
+  if (reject_impossible_fixit (src_range.m_start))
+    return;
+  if (reject_impossible_fixit (src_range.m_finish))
+    return;
+
+  /* Consolidate neighboring fixits.  */
+  fixit_hint *prev = get_last_fixit_hint ();
+  if (m_num_fixit_hints > 0)
+    {
+      if (prev->maybe_append_replace (m_line_table, src_range, new_content))
+	return;
+    }
+
   m_fixit_hints[m_num_fixit_hints++]
     = new fixit_replace (src_range, new_content);
 }
 
+/* Get the last fix-it hint within this rich_location, or NULL if none.  */
+
+fixit_hint *
+rich_location::get_last_fixit_hint () const
+{
+  if (m_num_fixit_hints > 0)
+    return m_fixit_hints[m_num_fixit_hints - 1];
+  else
+    return NULL;
+}
+
+/* If WHERE is an "awkward" location, then mark this rich_location as not
+   supporting fixits, purging any thay were already added, and return true.
+
+   Otherwise (the common case), return false.  */
+
+bool
+rich_location::reject_impossible_fixit (source_location where)
+{
+  /* Fix-its within a rich_location should either all be suggested, or
+     none of them should be suggested.
+     Once we've rejected a fixit, we reject any more, even those
+     with reasonable locations.  */
+  if (m_seen_impossible_fixit)
+    return true;
+
+  if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
+    /* WHERE is a reasonable location for a fix-it; don't reject it.  */
+    return false;
+
+  /* Otherwise we have an attempt to add a fix-it with an "awkward"
+     location: either one that we can't obtain column information
+     for (within an ordinary map), or one within a macro expansion.  */
+  m_seen_impossible_fixit = true;
+
+  /* Purge the rich_location of any fix-its that were already added. */
+  for (unsigned int i = 0; i < m_num_fixit_hints; i++)
+    delete m_fixit_hints[i];
+  m_num_fixit_hints = 0;
+
+  return true;
+}
+
 /* class fixit_insert.  */
 
 fixit_insert::fixit_insert (source_location where,
@@ -2129,6 +2229,15 @@ fixit_insert::affects_line_p (const char *file, int line)
   return false;
 }
 
+/* Implementation of maybe_append_replace for fixit_insert.  Reject
+   the attempt to consolidate fix-its.  */
+
+bool
+fixit_insert::maybe_append_replace (line_maps *, source_range, const char *)
+{
+  return false;
+}
+
 /* class fixit_replace.  */
 
 fixit_replace::fixit_replace (source_range src_range,
@@ -2151,3 +2260,26 @@ fixit_replace::affects_line_p (const char *file, int line)
 {
   return m_src_range.intersects_line_p (file, line);
 }
+
+/* Implementation of maybe_append_replace for fixit_replace.  If
+   possible, merge the new replacement into this one and return true.
+   Otherwise return false.  */
+
+bool
+fixit_replace::maybe_append_replace (line_maps *set,
+				     source_range src_range,
+				     const char *new_content)
+{
+  /* Does SRC_RANGE start immediately after this one finishes?  */
+  if (!column_before_p (set, m_src_range.m_finish, src_range.m_start))
+    return false;
+
+  /* We have neighboring replacements; merge them.  */
+  m_src_range.m_finish = src_range.m_finish;
+  size_t extra_len = strlen (new_content);
+  m_bytes = (char *)xrealloc (m_bytes, m_len + extra_len + 1);
+  memcpy (m_bytes + m_len, new_content, extra_len);
+  m_len += extra_len;
+  m_bytes[m_len] = '\0';
+  return true;
+}
-- 
1.8.5.3

Re: Add fixit hint for -Wlogical-not-parentheses

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[Marek Polacek]

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

Re: Add fixit hint for -Wlogical-not-parentheses

[David Malcolm]

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

[committed] make_location: ensure end-points are pure locations

[David Malcolm]

On Mon, 2016-08-29 at 08:50 -0400, David Malcolm wrote:
> 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.

The issue affected the C++ frontend, but not the C frontend.

The root cause was that a location expressing a packed range was
passed to a make_location call as the "start" parameter.
make_location was converting the caret to a pure location (stripping
away the packed range), but wasn't doing this for the "start" parameter.
This meant that caret != start, despite them being the same
file/line/col, due to the latter containing packed range information.
This was handled by make_location by creating an ad-hoc location, and
fixit-validation doesn't yet handle ad-hoc locations for insertion
fixits, only pure locations.

The following patch fixes the issue seen in this specific case by
fixing make_location to avoid storing ranges for the endpoints of a
range, and thus avoid generating ad-hoc locations.  If a location
containing a range is passed as start or finish to make_location, it
will use the start/finish of that range respectively.

I'm working on a followup fix to make the fixit-validation code
better handle any ad-hoc locations that do make it through.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu; converts
the failures in g++.sum into passes.

Committed to trunk as r239831.

gcc/ChangeLog:
	* input.c (make_location): Call get_start and get_finish
	on the endpoints to avoid storing packed ranges or ad-hoc
	ranges in them.
	(selftest::test_make_location_nonpure_range_endpoints): New function.
	(selftest::input_c_tests): Call it.
	* input.h (get_start): New inline function.
---
 gcc/input.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gcc/input.h |  8 ++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/gcc/input.c b/gcc/input.c
index 2dcecfb..4ec218d 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -879,8 +879,8 @@ make_location (location_t caret, location_t start, location_t finish)
 {
   location_t pure_loc = get_pure_location (caret);
   source_range src_range;
-  src_range.m_start = start;
-  src_range.m_finish = finish;
+  src_range.m_start = get_start (start);
+  src_range.m_finish = get_finish (finish);
   location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
 						   pure_loc,
 						   src_range,
@@ -1741,6 +1741,67 @@ test_builtins ()
   ASSERT_PRED1 (is_location_from_builtin_token, BUILTINS_LOCATION);
 }
 
+/* Regression test for make_location.
+   Ensure that we use the caret locations of the start/finish, rather
+   than storing a packed or ad-hoc range as the start/finish.  */
+
+static void
+test_make_location_nonpure_range_endpoints (const line_table_case &case_)
+{
+  /* Issue seen with testsuite/c-c++-common/Wlogical-not-parentheses-2.c
+     with C++ frontend.
+     ....................0000000001111111111222.
+     ....................1234567890123456789012.  */
+  const char *content = "     r += !aaa == bbb;\n";
+  temp_source_file tmp (SELFTEST_LOCATION, ".C", content);
+  line_table_test ltt (case_);
+  linemap_add (line_table, LC_ENTER, false, tmp.get_filename (), 1);
+
+  const location_t c11 = linemap_position_for_column (line_table, 11);
+  const location_t c12 = linemap_position_for_column (line_table, 12);
+  const location_t c13 = linemap_position_for_column (line_table, 13);
+  const location_t c14 = linemap_position_for_column (line_table, 14);
+  const location_t c21 = linemap_position_for_column (line_table, 21);
+
+  if (c21 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+    return;
+
+  /* Use column 13 for the caret location, arbitrarily, to verify that we
+     handle start != caret.  */
+  const location_t aaa = make_location (c13, c12, c14);
+  ASSERT_EQ (c13, get_pure_location (aaa));
+  ASSERT_EQ (c12, get_start (aaa));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_start (aaa)));
+  ASSERT_EQ (c14, get_finish (aaa));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_finish (aaa)));
+
+  /* Make a location using a location with a range as the start-point.  */
+  const location_t not_aaa = make_location (c11, aaa, c14);
+  ASSERT_EQ (c11, get_pure_location (not_aaa));
+  /* It should use the start location of the range, not store the range
+     itself.  */
+  ASSERT_EQ (c12, get_start (not_aaa));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_start (not_aaa)));
+  ASSERT_EQ (c14, get_finish (not_aaa));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_finish (not_aaa)));
+
+  /* Similarly, make a location with a range as the end-point.  */
+  const location_t aaa_eq_bbb = make_location (c12, c12, c21);
+  ASSERT_EQ (c12, get_pure_location (aaa_eq_bbb));
+  ASSERT_EQ (c12, get_start (aaa_eq_bbb));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_start (aaa_eq_bbb)));
+  ASSERT_EQ (c21, get_finish (aaa_eq_bbb));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_finish (aaa_eq_bbb)));
+  const location_t not_aaa_eq_bbb = make_location (c11, c12, aaa_eq_bbb);
+  /* It should use the finish location of the range, not store the range
+     itself.  */
+  ASSERT_EQ (c11, get_pure_location (not_aaa_eq_bbb));
+  ASSERT_EQ (c12, get_start (not_aaa_eq_bbb));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_start (not_aaa_eq_bbb)));
+  ASSERT_EQ (c21, get_finish (not_aaa_eq_bbb));
+  ASSERT_FALSE (IS_ADHOC_LOC (get_finish (not_aaa_eq_bbb)));
+}
+
 /* Verify reading of input files (e.g. for caret-based diagnostics).  */
 
 static void
@@ -3187,6 +3248,7 @@ input_c_tests ()
   test_should_have_column_data_p ();
   test_unknown_location ();
   test_builtins ();
+  for_each_line_table_case (test_make_location_nonpure_range_endpoints);
 
   for_each_line_table_case (test_accessing_ordinary_linemaps);
   for_each_line_table_case (test_lexer);
diff --git a/gcc/input.h b/gcc/input.h
index c99abfd..ecf8db3 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -79,6 +79,14 @@ extern location_t input_location;
 
 extern location_t get_pure_location (location_t loc);
 
+/* Get the start of any range encoded within location LOC.  */
+
+static inline location_t
+get_start (location_t loc)
+{
+  return get_range_from_loc (line_table, loc).m_start;
+}
+
 /* Get the endpoint of any range encoded within location LOC.  */
 
 static inline location_t
-- 
1.8.5.3

[committed] Allow the use of ad-hoc locations for fix-it hints

[David Malcolm]

On Mon, 2016-08-29 at 13:43 -0400, David Malcolm wrote:
> On Mon, 2016-08-29 at 08:50 -0400, David Malcolm wrote:
> > 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.
> 
> The issue affected the C++ frontend, but not the C frontend.
> 
> The root cause was that a location expressing a packed range was
> passed to a make_location call as the "start" parameter.
> make_location was converting the caret to a pure location (stripping
> away the packed range), but wasn't doing this for the "start"
> parameter.
> This meant that caret != start, despite them being the same
> file/line/col, due to the latter containing packed range information.
> This was handled by make_location by creating an ad-hoc location, and
> fixit-validation doesn't yet handle ad-hoc locations for insertion
> fixits, only pure locations.
> 
> The following patch fixes the issue seen in this specific case by
> fixing make_location to avoid storing ranges for the endpoints of a
> range, and thus avoid generating ad-hoc locations.  If a location
> containing a range is passed as start or finish to make_location, it
> will use the start/finish of that range respectively.
> 
> I'm working on a followup fix to make the fixit-validation code
> better handle any ad-hoc locations that do make it through.

Currently the fix-it validator rejects ad-hoc locations.
Fix this by calling get_pure_location on the input locations to
add_fixit_insert/replace.  Doing so requires moving get_pure_location
from gcc to libcpp.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

Committed to trunk as r239843.

gcc/ChangeLog:
	* diagnostic-show-locus.c
	(selftest::test_one_liner_fixit_validation_adhoc_locations): New
	function.
	(selftest::test_diagnostic_show_locus_one_liner): Call it.
	* input.c (get_pure_location): Move to libcpp/line-map.c.
	* input.h (get_pure_location): Convert decl to an inline function
	calling implementation in libcpp.

libcpp/ChangeLog:
	* include/line-map.h (get_pure_location): New decl.
	* line-map.c (get_pure_location): Move here, from gcc/input.c, adding
	a line_maps * param.
	(rich_location::add_fixit_insert): Call get_pure_location on "where".
	(rich_location::add_fixit_replace): Call get_pure_location on the
	end-points.
---
 gcc/diagnostic-show-locus.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
 gcc/input.c                 | 22 --------------
 gcc/input.h                 |  6 +++-
 libcpp/include/line-map.h   |  6 ++++
 libcpp/line-map.c           | 27 +++++++++++++++++
 5 files changed, 108 insertions(+), 23 deletions(-)

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index f3f661e..ba52f24 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1594,6 +1594,75 @@ test_one_liner_fixit_replace_equal_secondary_range ()
 		pp_formatted_text (dc.printer));
 }
 
+/* Verify that we can use ad-hoc locations when adding fixits to a
+   rich_location.  */
+
+static void
+test_one_liner_fixit_validation_adhoc_locations ()
+{
+  /* Generate a range that's too long to be packed, so must
+     be stored as an ad-hoc location (given the defaults
+     of 5 bits or 0 bits of packed range); 41 columns > 2**5.  */
+  const location_t c7 = linemap_position_for_column (line_table, 7);
+  const location_t c47 = linemap_position_for_column (line_table, 47);
+  const location_t loc = make_location (c7, c7, c47);
+
+  if (c47 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+    return;
+
+  ASSERT_TRUE (IS_ADHOC_LOC (loc));
+
+  /* Insert.  */
+  {
+    rich_location richloc (line_table, loc);
+    richloc.add_fixit_insert (loc, "test");
+    /* It should not have been discarded by the validator.  */
+    ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+    test_diagnostic_context dc;
+    diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+    ASSERT_STREQ ("\n"
+		  " foo = bar.field;\n"
+		  "       ^~~~~~~~~~                               \n"
+		  "       test\n",
+		  pp_formatted_text (dc.printer));
+  }
+
+  /* Remove.  */
+  {
+    rich_location richloc (line_table, loc);
+    source_range range = source_range::from_locations (loc, c47);
+    richloc.add_fixit_remove (range);
+    /* It should not have been discarded by the validator.  */
+    ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+    test_diagnostic_context dc;
+    diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+    ASSERT_STREQ ("\n"
+		  " foo = bar.field;\n"
+		  "       ^~~~~~~~~~                               \n"
+		  "       -----------------------------------------\n",
+		  pp_formatted_text (dc.printer));
+  }
+
+  /* Replace.  */
+  {
+    rich_location richloc (line_table, loc);
+    source_range range = source_range::from_locations (loc, c47);
+    richloc.add_fixit_replace (range, "test");
+    /* It should not have been discarded by the validator.  */
+    ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+    test_diagnostic_context dc;
+    diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+    ASSERT_STREQ ("\n"
+		  " foo = bar.field;\n"
+		  "       ^~~~~~~~~~                               \n"
+		  "       test\n",
+		  pp_formatted_text (dc.printer));
+  }
+}
+
 /* Run the various one-liner tests.  */
 
 static void
@@ -1626,6 +1695,7 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
   test_one_liner_fixit_replace ();
   test_one_liner_fixit_replace_non_equal_range ();
   test_one_liner_fixit_replace_equal_secondary_range ();
+  test_one_liner_fixit_validation_adhoc_locations ();
 }
 
 /* Verify that fix-it hints are appropriately consolidated.
diff --git a/gcc/input.c b/gcc/input.c
index 4ec218d..a3fe542 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -838,28 +838,6 @@ expansion_point_location (source_location location)
 				   LRK_MACRO_EXPANSION_POINT, NULL);
 }
 
-/* Given location LOC, strip away any packed range information
-   or ad-hoc information.  */
-
-location_t
-get_pure_location (location_t loc)
-{
-  if (IS_ADHOC_LOC (loc))
-    loc
-      = line_table->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].locus;
-
-  if (loc >= LINEMAPS_MACRO_LOWEST_LOCATION (line_table))
-    return loc;
-
-  if (loc < RESERVED_LOCATION_COUNT)
-    return loc;
-
-  const line_map *map = linemap_lookup (line_table, loc);
-  const line_map_ordinary *ordmap = linemap_check_ordinary (map);
-
-  return loc & ~((1 << ordmap->m_range_bits) - 1);
-}
-
 /* Construct a location with caret at CARET, ranging from START to
    finish e.g.
 
diff --git a/gcc/input.h b/gcc/input.h
index ecf8db3..fd21f34 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -77,7 +77,11 @@ extern location_t input_location;
 #define from_macro_expansion_at(LOC) \
   ((linemap_location_from_macro_expansion_p (line_table, LOC)))
 
-extern location_t get_pure_location (location_t loc);
+static inline location_t
+get_pure_location (location_t loc)
+{
+  return get_pure_location (line_table, loc);
+}
 
 /* Get the start of any range encoded within location LOC.  */
 
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 0fc4848..d9c31de 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1002,6 +1002,12 @@ IS_ADHOC_LOC (source_location loc)
 bool
 pure_location_p (line_maps *set, source_location loc);
 
+/* Given location LOC within SET, strip away any packed range information
+   or ad-hoc information.  */
+
+extern source_location get_pure_location (line_maps *set,
+					  source_location loc);
+
 /* Combine LOC and BLOCK, giving a combined adhoc location.  */
 
 inline source_location
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 8fe48bd..f5b1586 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -311,6 +311,28 @@ pure_location_p (line_maps *set, source_location loc)
   return true;
 }
 
+/* Given location LOC within SET, strip away any packed range information
+   or ad-hoc information.  */
+
+source_location
+get_pure_location (line_maps *set, source_location loc)
+{
+  if (IS_ADHOC_LOC (loc))
+    loc
+      = set->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].locus;
+
+  if (loc >= LINEMAPS_MACRO_LOWEST_LOCATION (set))
+    return loc;
+
+  if (loc < RESERVED_LOCATION_COUNT)
+    return loc;
+
+  const line_map *map = linemap_lookup (set, loc);
+  const line_map_ordinary *ordmap = linemap_check_ordinary (map);
+
+  return loc & ~((1 << ordmap->m_range_bits) - 1);
+}
+
 /* Finalize the location_adhoc_data structure.  */
 void
 location_adhoc_data_fini (struct line_maps *set)
@@ -2077,6 +2099,8 @@ void
 rich_location::add_fixit_insert (source_location where,
 				 const char *new_content)
 {
+  where = get_pure_location (m_line_table, where);
+
   if (reject_impossible_fixit (where))
     return;
 
@@ -2141,6 +2165,9 @@ rich_location::add_fixit_replace (source_range src_range,
 {
   linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
 
+  src_range.m_start = get_pure_location (m_line_table, src_range.m_start);
+  src_range.m_finish = get_pure_location (m_line_table, src_range.m_finish);
+
   if (reject_impossible_fixit (src_range.m_start))
     return;
   if (reject_impossible_fixit (src_range.m_finish))
-- 
1.8.5.3