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


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

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


On Wed, 2016-08-24 at 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


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