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 <pola...@redhat.com>
> David Malcolm <dmalc...@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
