c: Improve build_component_ref diagnostics [PR91134]
Commit Message
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.
* 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.
Jakub
Comments
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
@@ -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);
@@ -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)
@@ -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:
@@ -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)
@@ -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
}
@@ -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;
+}