Re: [PATCH 2/2] v2: C++: improvements to binary operator diagnostics (PR c++/87504)

[Jason Merrill <jason@redhat.com>]

On 12/4/18 5:35 PM, David Malcolm wrote:
> The v1 patch:
>    https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00303.html
> has bitrotten somewhat, so here's v2 of the patch, updated relative
> to r266740.
> 
> Blurb from v1 patch follows:
> 
> The C frontend is able (where expression locations are available) to print
> problems with binary operators in 3-location form, labelling the types of
> the expressions:
> 
>    arg_0 op arg_1
>    ~~~~~ ^~ ~~~~~
>      |        |
>      |        arg1 type
>      arg0 type
> 
> The C++ frontend currently just shows the combined location:
> 
>    arg_0 op arg_1
>    ~~~~~~^~~~~~~~
> 
> and fails to highlight where the subexpressions are, or their types.
> 
> This patch introduces a op_location_t struct for handling the above
> operator-location vs combined-location split, and a new
> class binary_op_rich_location for displaying the above, so that the
> C++ frontend is able to use the more detailed 3-location form for
> type mismatches in binary operators, and for -Wtautological-compare
> (where types are not displayed).  Both forms can be seen in this
> example:
> 
> bad-binary-ops.C:69:20: error: no match for 'operator&&' (operand types are
>    's' and 't')
>     69 |   return ns_4::foo && ns_4::inner::bar;
>        |          ~~~~~~~~~ ^~ ~~~~~~~~~~~~~~~~
>        |                |                   |
>        |                s                   t
> bad-binary-ops.C:69:20: note: candidate: 'operator&&(bool, bool)' <built-in>
>     69 |   return ns_4::foo && ns_4::inner::bar;
>        |          ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> 
> The patch also allows from some uses of macros in
> -Wtautological-compare, where both sides of the comparison have
> been spelled the same way, e.g.:
> 
> Wtautological-compare-ranges.c:23:11: warning: self-comparison always
>     evaluates to true [-Wtautological-compare]
>     23 |   if (FOO == FOO);
>        |           ^~
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu, in
> conjunction with the previous patch.
> 
> OK for trunk?
> Dave
> 
> gcc/c-family/ChangeLog:
> 	PR c++/87504
> 	* c-common.h (warn_tautological_cmp): Convert 1st param from
> 	location_t to const op_location_t &.
> 	* c-warn.c (find_array_ref_with_const_idx_r): Strip location
> 	wrapper when testing for INTEGER_CST.
> 	(warn_tautological_bitwise_comparison): Convert 1st param from
> 	location_t to const op_location_t &; use it to build a
> 	binary_op_rich_location, and use this.
> 	(spelled_the_same_p): New function.
> 	(warn_tautological_cmp): Convert 1st param from location_t to
> 	const op_location_t &.  Warn for macro expansions if
> 	spelled_the_same_p.  Use binary_op_rich_location.
> 
> gcc/c/ChangeLog:
> 	PR c++/87504
> 	* c-typeck.c (class maybe_range_label_for_tree_type_mismatch):
> 	Move from here to gcc-rich-location.h and gcc-rich-location.c.
> 	(build_binary_op): Use struct op_location_t and
> 	class binary_op_rich_location.
> 
> gcc/cp/ChangeLog:
> 	PR c++/87504
> 	* call.c (op_error): Convert 1st param from location_t to
> 	const op_location_t &.  Use binary_op_rich_location for binary
> 	ops.
> 	(build_conditional_expr_1): Convert 1st param from location_t to
> 	const op_location_t &.
> 	(build_conditional_expr): Likewise.
> 	(build_new_op_1): Likewise.
> 	(build_new_op): Likewise.
> 	* cp-tree.h (build_conditional_expr): Likewise.
> 	(build_new_op): Likewise.
> 	(build_x_binary_op): Likewise.
> 	(cp_build_binary_op): Likewise.
> 	* parser.c (cp_parser_primary_expression): Build a location
> 	for id-expression nodes.
> 	(cp_parser_binary_expression): Use an op_location_t when
> 	calling build_x_binary_op.
> 	(cp_parser_operator): Build a location for user-defined literals.
> 	* typeck.c (build_x_binary_op): Convert 1st param from location_t
> 	to const op_location_t &.
> 	(cp_build_binary_op): Likewise.  Use binary_op_rich_location.
> 
> gcc/ChangeLog:
> 	PR c++/87504
> 	* gcc-rich-location.c
> 	(maybe_range_label_for_tree_type_mismatch::get_text): Move here from
> 	c/c-typeck.c.
> 	(binary_op_rich_location::binary_op_rich_location): New ctor.
> 	(binary_op_rich_location::use_operator_loc_p): New function.
> 	* gcc-rich-location.h
> 	(class maybe_range_label_for_tree_type_mismatch)): Move here from
> 	c/c-typeck.c.
> 	(struct op_location_t): New forward decl.
> 	(class binary_op_rich_location): New class.
> 	* tree.h (struct op_location_t): New struct.
> 
> gcc/testsuite/ChangeLog:
> 	* c-c++-common/Wtautological-compare-ranges.c: New test.
> 	* g++.dg/cpp0x/pr51420.C: Add -fdiagnostics-show-caret and update
> 	expected output.
> 	* g++.dg/diagnostic/bad-binary-ops.C: Update expected output from
> 	1-location form to 3-location form, with labelling of ranges with
> 	types.  Add examples of id-expression nodes with namespaces.
> 	* g++.dg/diagnostic/param-type-mismatch-2.C: Likewise.
> 
> This is the 2nd commit message:
> 
> FIXME: column and multiline fixes to * g++.dg/cpp0x/pr51420.C
> ---
>   gcc/c-family/c-common.h                            |  3 +-
>   gcc/c-family/c-warn.c                              | 57 +++++++++++---
>   gcc/c/c-typeck.c                                   | 41 +---------
>   gcc/cp/call.c                                      | 28 ++++---
>   gcc/cp/cp-tree.h                                   | 10 ++-
>   gcc/cp/parser.c                                    | 32 ++++++--
>   gcc/cp/typeck.c                                    | 14 ++--
>   gcc/gcc-rich-location.c                            | 89 ++++++++++++++++++++++
>   gcc/gcc-rich-location.h                            | 57 ++++++++++++++
>   .../c-c++-common/Wtautological-compare-ranges.c    | 42 ++++++++++
>   gcc/testsuite/g++.dg/cpp0x/pr51420.C               | 10 +++
>   gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C   | 57 +++++++++++++-
>   .../g++.dg/diagnostic/param-type-mismatch-2.C      |  4 +-
>   gcc/tree.h                                         | 49 ++++++++++++
>   14 files changed, 417 insertions(+), 76 deletions(-)
>   create mode 100644 gcc/testsuite/c-c++-common/Wtautological-compare-ranges.c
> 
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index 4187343..0b9ddf6 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -1268,7 +1268,8 @@ extern void constant_expression_error (tree);
>   extern void overflow_warning (location_t, tree, tree = NULL_TREE);
>   extern void warn_logical_operator (location_t, enum tree_code, tree,
>   				   enum tree_code, tree, enum tree_code, tree);
> -extern void warn_tautological_cmp (location_t, enum tree_code, tree, tree);
> +extern void warn_tautological_cmp (const op_location_t &, enum tree_code,
> +				   tree, tree);
>   extern void warn_logical_not_parentheses (location_t, enum tree_code, tree,
>   					  tree);
>   extern bool warn_if_unused_value (const_tree, location_t);
> diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
> index fc7f87c..fce9d84 100644
> --- a/gcc/c-family/c-warn.c
> +++ b/gcc/c-family/c-warn.c
> @@ -322,7 +322,8 @@ find_array_ref_with_const_idx_r (tree *expr_p, int *, void *)
>   
>     if ((TREE_CODE (expr) == ARRAY_REF
>          || TREE_CODE (expr) == ARRAY_RANGE_REF)
> -      && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST)
> +      && (TREE_CODE (tree_strip_any_location_wrapper (TREE_OPERAND (expr, 1)))
> +	  == INTEGER_CST))
>       return integer_type_node;

I think we want fold_for_warn here.  OK with that change (assuming it 
passes).

Jason