On Tue, May 24, 2022 at 09:25:57AM +0200, Jakub Jelinek wrote:
> Hi!
>
> On the following testcase (the first dg-error line) we emit a weird
> diagnostics and even fixit on pointerpointer->member
> where pointerpointer is pointer to pointer to struct and we say
> 'pointerpointer' is a pointer; did you mean to use '->'?
> The first part is indeed true, but suggesting -> when the code already
> does use -> is confusing.
> The following patch adjusts callers so that they tell it if it is from
> . parsing or from -> parsing and in the latter case suggests to dereference
> the left operand instead by adding (* before it and ) after it (before ->).
> Or would a suggestion to add [0] before -> be better?
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2022-05-24 Jakub Jelinek <ja...@redhat.com>
>
> PR c/91134
> gcc/c/
> * c-tree.h (build_component_ref): Add ARROW_LOC location_t argument.
> * c-typeck.cc (build_component_ref): Likewise. If DATUM is
> INDIRECT_REF and ARROW_LOC isn't UNKNOWN_LOCATION, print a different
> diagnostics and fixit hint if DATUM has pointer type.
s/diagnostic/, missing "than" before if?
> * c-parser.cc (c_parser_postfix_expression,
> c_parser_omp_variable_list): Adjust build_component_ref callers.
> * gimple-parser.cc (c_parser_gimple_postfix_expression_after_primary):
> Likewise.
> gcc/objc/
> * objc-act.cc (objc_build_component_ref): Adjust build_component_ref
> caller.
> gcc/testsuite/
> * gcc.dg/pr91134.c: New test.
>
> --- gcc/c/c-tree.h.jj 2022-05-19 11:48:56.058291437 +0200
> +++ gcc/c/c-tree.h 2022-05-23 20:22:05.669515990 +0200
> @@ -699,7 +699,8 @@ extern struct c_expr convert_lvalue_to_r
> extern tree decl_constant_value_1 (tree, bool);
> extern void mark_exp_read (tree);
> extern tree composite_type (tree, tree);
> -extern tree build_component_ref (location_t, tree, tree, location_t);
> +extern tree build_component_ref (location_t, tree, tree, location_t,
> + location_t);
> extern tree build_array_ref (location_t, tree, tree);
> extern tree build_external_ref (location_t, tree, bool, tree *);
> extern void pop_maybe_used (bool);
> --- gcc/c/c-typeck.cc.jj 2022-05-19 11:48:56.077291176 +0200
> +++ gcc/c/c-typeck.cc 2022-05-23 20:23:44.713515875 +0200
> @@ -2457,11 +2457,12 @@ should_suggest_deref_p (tree datum_type)
> /* Make an expression to refer to the COMPONENT field of structure or
> union value DATUM. COMPONENT is an IDENTIFIER_NODE. LOC is the
> location of the COMPONENT_REF. COMPONENT_LOC is the location
> - of COMPONENT. */
> + of COMPONENT. ARROW_LOC is the location of first -> operand if
"of the first"?
> + it is from -> operator. */
>
> tree
> build_component_ref (location_t loc, tree datum, tree component,
> - location_t component_loc)
> + location_t component_loc, location_t arrow_loc)
> {
> tree type = TREE_TYPE (datum);
> enum tree_code code = TREE_CODE (type);
> @@ -2577,11 +2578,23 @@ build_component_ref (location_t loc, tre
> /* Special-case the error message for "ptr.field" for the case
> where the user has confused "." vs "->". */
> rich_location richloc (line_table, loc);
> - /* "loc" should be the "." token. */
> - richloc.add_fixit_replace ("->");
> - error_at (&richloc,
> - "%qE is a pointer; did you mean to use %<->%>?",
> - datum);
> + if (TREE_CODE (datum) == INDIRECT_REF && arrow_loc != UNKNOWN_LOCATION)
> + {
> + richloc.add_fixit_insert_before (arrow_loc, "(*");
> + richloc.add_fixit_insert_after (arrow_loc, ")");
> + error_at (&richloc,
> + "%qE is a pointer to pointer; did you mean to dereference "
> + "it before applying %<->%> to it?",
> + TREE_OPERAND (datum, 0));
> + }
> + else
> + {
> + /* "loc" should be the "." token. */
> + richloc.add_fixit_replace ("->");
> + error_at (&richloc,
> + "%qE is a pointer; did you mean to use %<->%>?",
> + datum);
> + }
> return error_mark_node;
> }
> else if (code != ERROR_MARK)
> --- gcc/c/c-parser.cc.jj 2022-05-23 16:16:30.360856580 +0200
> +++ gcc/c/c-parser.cc 2022-05-23 20:33:36.683537409 +0200
> @@ -9235,8 +9235,9 @@ c_parser_postfix_expression (c_parser *p
> if (c_parser_next_token_is (parser, CPP_NAME))
> {
> c_token *comp_tok = c_parser_peek_token (parser);
> - offsetof_ref = build_component_ref
> - (loc, offsetof_ref, comp_tok->value, comp_tok->location);
> + offsetof_ref
> + = build_component_ref (loc, offsetof_ref, comp_tok->value,
> + comp_tok->location, UNKNOWN_LOCATION);
> c_parser_consume_token (parser);
> while (c_parser_next_token_is (parser, CPP_DOT)
> || c_parser_next_token_is (parser,
> @@ -9263,9 +9264,11 @@ c_parser_postfix_expression (c_parser *p
> break;
> }
> c_token *comp_tok = c_parser_peek_token (parser);
> - offsetof_ref = build_component_ref
> - (loc, offsetof_ref, comp_tok->value,
> - comp_tok->location);
> + offsetof_ref
> + = build_component_ref (loc, offsetof_ref,
> + comp_tok->value,
> + comp_tok->location,
> + UNKNOWN_LOCATION);
> c_parser_consume_token (parser);
> }
> else
> @@ -10612,7 +10615,7 @@ c_parser_postfix_expression_after_primar
> finish = c_parser_peek_token (parser)->get_finish ();
> c_parser_consume_token (parser);
> expr.value = build_component_ref (op_loc, expr.value, ident,
> - comp_loc);
> + comp_loc, UNKNOWN_LOCATION);
> set_c_expr_source_range (&expr, start, finish);
> expr.original_code = ERROR_MARK;
> if (TREE_CODE (expr.value) != COMPONENT_REF)
> @@ -10652,7 +10655,8 @@ c_parser_postfix_expression_after_primar
> build_indirect_ref (op_loc,
> expr.value,
> RO_ARROW),
> - ident, comp_loc);
> + ident, comp_loc,
> + expr.get_location ());
> set_c_expr_source_range (&expr, start, finish);
> expr.original_code = ERROR_MARK;
> if (TREE_CODE (expr.value) != COMPONENT_REF)
> @@ -13171,6 +13175,7 @@ c_parser_omp_variable_list (c_parser *pa
> && c_parser_next_token_is (parser, CPP_DEREF)))
> {
> location_t op_loc = c_parser_peek_token (parser)->location;
> + location_t arrow_loc = UNKNOWN_LOCATION;
> if (c_parser_next_token_is (parser, CPP_DEREF))
> {
> c_expr t_expr;
> @@ -13181,6 +13186,7 @@ c_parser_omp_variable_list (c_parser *pa
> t_expr = convert_lvalue_to_rvalue (op_loc, t_expr,
> true, false);
> t = build_indirect_ref (op_loc, t_expr.value, RO_ARROW);
> + arrow_loc = t_expr.get_location ();
> }
> c_parser_consume_token (parser);
> if (!c_parser_next_token_is (parser, CPP_NAME))
> @@ -13194,7 +13200,8 @@ c_parser_omp_variable_list (c_parser *pa
> tree ident = comp_tok->value;
> location_t comp_loc = comp_tok->location;
> c_parser_consume_token (parser);
> - t = build_component_ref (op_loc, t, ident, comp_loc);
> + t = build_component_ref (op_loc, t, ident, comp_loc,
> + arrow_loc);
> }
> /* FALLTHROUGH */
> case OMP_CLAUSE_AFFINITY:
> --- gcc/c/gimple-parser.cc.jj 2022-02-24 15:27:14.718744040 +0100
> +++ gcc/c/gimple-parser.cc 2022-05-23 20:27:34.538195175 +0200
> @@ -1800,7 +1800,7 @@ c_parser_gimple_postfix_expression_after
> finish = c_parser_peek_token (parser)->get_finish ();
> c_parser_consume_token (parser);
> expr.value = build_component_ref (op_loc, expr.value, ident,
> - comp_loc);
> + comp_loc, UNKNOWN_LOCATION);
> set_c_expr_source_range (&expr, start, finish);
> expr.original_code = ERROR_MARK;
> if (TREE_CODE (expr.value) != COMPONENT_REF)
> @@ -1848,7 +1848,8 @@ c_parser_gimple_postfix_expression_after
> expr.value = build_component_ref (op_loc,
> build_simple_mem_ref_loc
> (op_loc, expr.value),
> - ident, comp_loc);
> + ident, comp_loc,
> + expr.get_location ());
> set_c_expr_source_range (&expr, start, finish);
> expr.original_code = ERROR_MARK;
> if (TREE_CODE (expr.value) != COMPONENT_REF)
> --- gcc/objc/objc-act.cc.jj 2022-01-18 00:18:02.827743339 +0100
> +++ gcc/objc/objc-act.cc 2022-05-23 23:59:48.134923529 +0200
> @@ -2812,7 +2812,7 @@ objc_build_component_ref (tree datum, tr
> tf_warning_or_error);
> #else
> return build_component_ref (input_location, datum, component,
> - UNKNOWN_LOCATION);
> + UNKNOWN_LOCATION, UNKNOWN_LOCATION);
> #endif
> }
>
> --- gcc/testsuite/gcc.dg/pr91134.c.jj 2022-05-23 20:31:11.751001817 +0200
> +++ gcc/testsuite/gcc.dg/pr91134.c 2022-05-23 20:30:45.291268997 +0200
> @@ -0,0 +1,13 @@
> +/* PR c/91134 */
> +
> +struct X { int member; } x;
> +
> +int
> +foo (void)
> +{
> + struct X *pointer = &x;
> + struct X **pointerpointer = &pointer;
> + int i = *pointerpointer->member; /* { dg-error "'pointerpointer' is a
> pointer to pointer; did you mean to dereference it before applying '->' to
> it\\\?" } */
> + int j = pointer.member; /* { dg-error "'pointer' is a pointer;
> did you mean to use '->'\\\?" } */
> + return i + j;
> +}
Consider extending the test like
--- pr91134.c 2022-05-24 09:33:17.807520253 -0400
+++ pr911342.c 2022-05-24 09:36:09.908217139 -0400
@@ -1,13 +1,16 @@
/* PR c/91134 */
struct X { int member; } x;
+struct Y { struct X **x; } y;
int
foo (void)
{
struct X *pointer = &x;
+ struct Y *yp = &y;
struct X **pointerpointer = &pointer;
int i = *pointerpointer->member; /* { dg-error "'pointerpointer' is a
pointer to pointer; did you mean to dereference it before applying '->' to
it\\\?" } */
int j = pointer.member; /* { dg-error "'pointer' is a pointer;
did you mean to use '->'\\\?" } */
- return i + j;
+ int k = yp->x->member; // dg-error
+ return i + j + k;
}
but I think the patch is OK, thanks.
Marek
