Message ID | YoyIhfJbdPn9PgHH@tucnak |
---|---|
State | New |
Headers | show |
Series | c: Improve build_component_ref diagnostics [PR91134] | expand |
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 <jakub@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
On Tue, 2022-05-24 at 09:25 +0200, Jakub Jelinek via Gcc-patches 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? > [...snip implementation...] > > --- 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; > +} Ideally we'd have an automated check that the fix-it hint fixes the code, but failing that, I like to have at least some DejaGnu test coverage for fix-it hints - something like the tests in gcc.dg/fixits.c or gcc.dg/semicolon-fixits.c, perhaps? Dave
On Tue, 2022-05-24 at 09:57 -0400, David Malcolm wrote: > On Tue, 2022-05-24 at 09:25 +0200, Jakub Jelinek via Gcc-patches > 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? > > > > [...snip implementation...] > > > > > --- 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; > > +} > > Ideally we'd have an automated check that the fix-it hint fixes the > code, but failing that, I like to have at least some DejaGnu test > coverage for fix-it hints - something like the tests in > gcc.dg/fixits.c > or gcc.dg/semicolon-fixits.c, perhaps? Also, what does the output from: -fdiagnostics-generate-patch look like? That's usually the best way of checking if we're generating good fix-it hints. Dave
On Tue, May 24, 2022 at 09:43:54AM -0400, Marek Polacek wrote:
> Consider extending the test like
Done, + added the dg-*-multiline-output stuff to test the fixit hints.
Thanks.
Here is what I've committed:
2022-05-25 Jakub Jelinek <jakub@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
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-tree.h.jj 2022-05-25 11:06:58.381515989 +0200
+++ gcc/c/c-tree.h 2022-05-25 13:57:58.975539087 +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-25 11:06:58.388515915 +0200
+++ gcc/c/c-typeck.cc 2022-05-25 14:19:16.486336943 +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 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, 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-25 11:06:58.355516262 +0200
+++ gcc/c/c-parser.cc 2022-05-25 13:57:58.980539036 +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-05-24 08:59:33.375472523 +0200
+++ gcc/c/gimple-parser.cc 2022-05-25 13:57:59.000538828 +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 11:58:59.693980471 +0100
+++ gcc/objc/objc-act.cc 2022-05-25 13:57:59.018538641 +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-25 13:57:59.018538641 +0200
+++ gcc/testsuite/gcc.dg/pr91134.c 2022-05-25 14:17:17.000576752 +0200
@@ -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;
+}
Jakub
On Tue, May 24, 2022 at 09:59:03AM -0400, David Malcolm wrote: > > Ideally we'd have an automated check that the fix-it hint fixes the > > code, but failing that, I like to have at least some DejaGnu test > > coverage for fix-it hints - something like the tests in > > gcc.dg/fixits.c > > or gcc.dg/semicolon-fixits.c, perhaps? Done, see another mail. > Also, what does the output from: > -fdiagnostics-generate-patch > look like? That's usually the best way of checking if we're generating > good fix-it hints. --- pr91134.c +++ pr91134.c @@ -10,19 +10,19 @@ 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 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 '->'\\\?" } */ + 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\\\?" } */ + 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; ^~ The second and third change actually fix those issues and make it compile, the first one will generate a different error, because the right change in that case is int i = (*pointerpointer)->member; but guessing that in the compiler would be too hard, when parsing pointerpointer->member we really don't know that there is * before it... Jakub
--- 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 + 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; +}