From 7a3ee77a2e33b8b8ad31aea27996ebe92a5c8d83 Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Wed, 25 May 2022 14:21:54 +0200 Subject: [PATCH] c: Improve build_component_ref diagnostics [PR91134] 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? 2022-05-25 Jakub Jelinek 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 diagnostic and fixit hint if DATUM has pointer type. * 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-parser.cc | 23 +++++++++++++++-------- gcc/c/c-tree.h | 3 ++- gcc/c/c-typeck.cc | 27 ++++++++++++++++++++------- gcc/c/gimple-parser.cc | 5 +++-- gcc/objc/objc-act.cc | 2 +- gcc/testsuite/gcc.dg/pr91134.c | 32 ++++++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr91134.c diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc index 492d995..aa24b43 100644 --- a/gcc/c/c-parser.cc +++ b/gcc/c/c-parser.cc @@ -9235,8 +9235,9 @@ c_parser_postfix_expression (c_parser *parser) 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 *parser) 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_primary (c_parser *parser, 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_primary (c_parser *parser, 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 *parser, && 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 *parser, 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 *parser, 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: diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 2bcb966..3b322ad 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -699,7 +699,8 @@ extern struct c_expr convert_lvalue_to_rvalue (location_t, struct c_expr, 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); diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index 4f3611f..6b05edd 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -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 the first -> operand if + 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, tree datum, tree component, /* 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) diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc index d1afd42..b909eac 100644 --- a/gcc/c/gimple-parser.cc +++ b/gcc/c/gimple-parser.cc @@ -1800,7 +1800,7 @@ c_parser_gimple_postfix_expression_after_primary (gimple_parser &parser, 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_primary (gimple_parser &parser, 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) diff --git a/gcc/objc/objc-act.cc b/gcc/objc/objc-act.cc index 252274c..10591bb 100644 --- a/gcc/objc/objc-act.cc +++ b/gcc/objc/objc-act.cc @@ -2812,7 +2812,7 @@ objc_build_component_ref (tree datum, tree component) tf_warning_or_error); #else return build_component_ref (input_location, datum, component, - UNKNOWN_LOCATION); + UNKNOWN_LOCATION, UNKNOWN_LOCATION); #endif } diff --git a/gcc/testsuite/gcc.dg/pr91134.c b/gcc/testsuite/gcc.dg/pr91134.c new file mode 100644 index 0000000..8844f42 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr91134.c @@ -0,0 +1,32 @@ +/* PR c/91134 */ +/* { dg-options "-fdiagnostics-show-caret" } */ + +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\\\?" } */ +/* { dg-begin-multiline-output "" } + int i = *pointerpointer->member; + ^~ + (* ) + { dg-end-multiline-output "" } */ + int j = pointer.member; /* { dg-error "'pointer' is a pointer; did you mean to use '->'\\\?" } */ +/* { dg-begin-multiline-output "" } + int j = pointer.member; + ^ + -> + { dg-end-multiline-output "" } */ + int k = yp->x->member; /* { dg-error "'yp->x' is a pointer to pointer; did you mean to dereference it before applying '->' to it\\\?" } */ +/* { dg-begin-multiline-output "" } + int k = yp->x->member; + ^~ + (* ) + { dg-end-multiline-output "" } */ + return i + j + k; +} -- 2.7.4