diff mbox series

c++: Don't show constructor internal name in error message [PR105483]

Message ID 010201918f97e233-3b2c29a5-89f0-4cf6-8161-dded50bb5b00-000000@eu-west-1.amazonses.com
State New
Headers show
Series c++: Don't show constructor internal name in error message [PR105483] | expand

Commit Message

Simon Martin Aug. 26, 2024, 4:49 p.m. UTC 
We mention 'X::__ct' instead of 'X::X' in the "names the constructor,
not the type" error for this invalid code:

=== cut here ===
struct X {};
void g () {
  X::X x;
}
=== cut here ===

The problem is that we use %<%T::%D%> to build the error message, while
%qE does exactly what we need since we have DECL_CONSTRUCTOR_P. This is
what this patch does, along with skipping until the end of the statement
to avoid emitting extra (useless) errors.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/105483

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_expression_statement): Use %qE instead of
	incorrect %<%T::%D%>, and skip to end of statement.

gcc/testsuite/ChangeLog:

	* g++.dg/tc1/dr147.C: Adjust test expectation.
	* g++.dg/diagnostic/pr105483.C: New test.

---
 gcc/cp/parser.cc                           | 7 ++++---
 gcc/testsuite/g++.dg/diagnostic/pr105483.C | 7 +++++++
 gcc/testsuite/g++.dg/tc1/dr147.C           | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/diagnostic/pr105483.C

Comments

Jason Merrill Aug. 26, 2024, 5:30 p.m. UTC | #1 
On 8/26/24 12:49 PM, Simon Martin wrote:
> We mention 'X::__ct' instead of 'X::X' in the "names the constructor,
> not the type" error for this invalid code:
> 
> === cut here ===
> struct X {};
> void g () {
>    X::X x;
> }
> === cut here ===
> 
> The problem is that we use %<%T::%D%> to build the error message, while
> %qE does exactly what we need since we have DECL_CONSTRUCTOR_P. This is
> what this patch does, along with skipping until the end of the statement
> to avoid emitting extra (useless) errors.
> 
> Successfully tested on x86_64-pc-linux-gnu.
> 
> 	PR c++/105483
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.cc (cp_parser_expression_statement): Use %qE instead of
> 	incorrect %<%T::%D%>, and skip to end of statement.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/tc1/dr147.C: Adjust test expectation.
> 	* g++.dg/diagnostic/pr105483.C: New test.
> 
> ---
>   gcc/cp/parser.cc                           | 7 ++++---
>   gcc/testsuite/g++.dg/diagnostic/pr105483.C | 7 +++++++
>   gcc/testsuite/g++.dg/tc1/dr147.C           | 2 +-
>   3 files changed, 12 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/pr105483.C
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 28ebf2beb60..ef4e3838a86 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -13240,10 +13240,11 @@ cp_parser_expression_statement (cp_parser* parser, tree in_statement_expr)
>   	       && DECL_CONSTRUCTOR_P (get_first_fn (statement)))
>   	{
>   	  /* A::A a; */
> -	  tree fn = get_first_fn (statement);
>   	  error_at (token->location,
> -		    "%<%T::%D%> names the constructor, not the type",
> -		    DECL_CONTEXT (fn), DECL_NAME (fn));
> +		    "%qE names the constructor, not the type",
> +		    get_first_fn (statement));
> +	  cp_parser_skip_to_end_of_block_or_statement (parser);

Why block_or_statement rather than just _statement?

Maybe move the skip+return out of this block to share it with the 
preceding typename error?

Jason
Simon Martin Aug. 27, 2024, 5:15 p.m. UTC | #2 
Hi Jason,

On 26 Aug 2024, at 19:30, Jason Merrill wrote:

> On 8/26/24 12:49 PM, Simon Martin wrote:
>> We mention 'X::__ct' instead of 'X::X' in the "names the constructor,

>> not the type" error for this invalid code:
>>
>> === cut here ===
>> struct X {};
>> void g () {
>>    X::X x;
>> }
>> === cut here ===
>>
>> The problem is that we use %<%T::%D%> to build the error message, 
>> while
>> %qE does exactly what we need since we have DECL_CONSTRUCTOR_P. This 
>> is
>> what this patch does, along with skipping until the end of the 
>> statement
>> to avoid emitting extra (useless) errors.
>>
>> Successfully tested on x86_64-pc-linux-gnu.
>>
>> 	PR c++/105483
>>
>> gcc/cp/ChangeLog:
>>
>> 	* parser.cc (cp_parser_expression_statement): Use %qE instead of
>> 	incorrect %<%T::%D%>, and skip to end of statement.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/tc1/dr147.C: Adjust test expectation.
>> 	* g++.dg/diagnostic/pr105483.C: New test.
>>
>> ---
>>   gcc/cp/parser.cc                           | 7 ++++---
>>   gcc/testsuite/g++.dg/diagnostic/pr105483.C | 7 +++++++
>>   gcc/testsuite/g++.dg/tc1/dr147.C           | 2 +-
>>   3 files changed, 12 insertions(+), 4 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/pr105483.C
>>
>> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>> index 28ebf2beb60..ef4e3838a86 100644
>> --- a/gcc/cp/parser.cc
>> +++ b/gcc/cp/parser.cc
>> @@ -13240,10 +13240,11 @@ cp_parser_expression_statement (cp_parser* 
>> parser, tree in_statement_expr)
>>   	       && DECL_CONSTRUCTOR_P (get_first_fn (statement)))
>>   	{
>>   	  /* A::A a; */
>> -	  tree fn = get_first_fn (statement);
>>   	  error_at (token->location,
>> -		    "%<%T::%D%> names the constructor, not the type",
>> -		    DECL_CONTEXT (fn), DECL_NAME (fn));
>> +		    "%qE names the constructor, not the type",
>> +		    get_first_fn (statement));
>> +	  cp_parser_skip_to_end_of_block_or_statement (parser);
>
> Why block_or_statement rather than just _statement?
It was a mistake, thanks for catching it!

> Maybe move the skip+return out of this block to share it with the 
> preceding typename error?
Good idea. It’s a tiny bit more involved than just moving, because 
we’d miss genuine errors emitted by 
cp_parser_consume_semicolon_at_end_of_statement (e.g. break the 
c-c++-common/pr44515.c test, among others), however the updated patch 

does what you’re suggesting. I have successfully tested on 
x86_64-pc-linux-gnu. OK for trunk?

Thanks!
   Simon
>
> Jason
From 00b9f316f7d20f75b150c23fa4d4c9bdc02191b8 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Mon, 26 Aug 2024 14:09:46 +0200
Subject: [PATCH] c++: Don't show constructor internal name in error message [PR105483]

We mention 'X::__ct' instead of 'X::X' in the "names the constructor,
not the type" error for this invalid code:

=== cut here ===
struct X {};
void g () {
  X::X x;
}
=== cut here ===

The problem is that we use %<%T::%D%> to build the error message, while
%qE does exactly what we need since we have DECL_CONSTRUCTOR_P. This is
what this patch does.

It also skips until the end of the statement and returns error_mark_node
for this and the preceding if block, to avoid emitting extra (useless)
errors.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/105483

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_expression_statement): Use %qE instead of
	incorrect %<%T::%D%>. Skip to end of statement and return
	error_mark_node in case of error.

gcc/testsuite/ChangeLog:

	* g++.dg/parse/error36.C: Adjust test expectation.
	* g++.dg/tc1/dr147.C: Likewise.
	* g++.old-deja/g++.other/typename1.C: Likewise.
	* g++.dg/diagnostic/pr105483.C: New test.

---
 gcc/cp/parser.cc                                 | 14 +++++++++-----
 gcc/testsuite/g++.dg/diagnostic/pr105483.C       |  7 +++++++
 gcc/testsuite/g++.dg/parse/error36.C             |  4 ++--
 gcc/testsuite/g++.dg/tc1/dr147.C                 |  2 +-
 gcc/testsuite/g++.old-deja/g++.other/typename1.C |  2 +-
 5 files changed, 20 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/diagnostic/pr105483.C

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 28ebf2beb60..a722641be34 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -13232,18 +13232,22 @@ cp_parser_expression_statement (cp_parser* parser, tree in_statement_expr)
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_SEMICOLON)
       && !cp_parser_uncommitted_to_tentative_parse_p (parser))
     {
+      bool has_errored = true;
       if (TREE_CODE (statement) == SCOPE_REF)
 	error_at (token->location, "need %<typename%> before %qE because "
 		  "%qT is a dependent scope",
 		  statement, TREE_OPERAND (statement, 0));
       else if (is_overloaded_fn (statement)
 	       && DECL_CONSTRUCTOR_P (get_first_fn (statement)))
+	/* A::A a; */
+	error_at (token->location, "%qE names the constructor, not the type",
+		  get_first_fn (statement));
+      else
+	has_errored = false;
+      if (has_errored)
 	{
-	  /* A::A a; */
-	  tree fn = get_first_fn (statement);
-	  error_at (token->location,
-		    "%<%T::%D%> names the constructor, not the type",
-		    DECL_CONTEXT (fn), DECL_NAME (fn));
+	  cp_parser_skip_to_end_of_statement (parser);
+	  return error_mark_node;
 	}
     }
 
diff --git a/gcc/testsuite/g++.dg/diagnostic/pr105483.C b/gcc/testsuite/g++.dg/diagnostic/pr105483.C
new file mode 100644
index 00000000000..b935bacea11
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/pr105483.C
@@ -0,0 +1,7 @@
+// PR c++/105483
+// { dg-do compile }
+
+struct X { };
+void g () {
+  X::X x; // { dg-error "'X::X' names the constructor" }
+}
diff --git a/gcc/testsuite/g++.dg/parse/error36.C b/gcc/testsuite/g++.dg/parse/error36.C
index bf57a6b68ce..73b550e3f2a 100644
--- a/gcc/testsuite/g++.dg/parse/error36.C
+++ b/gcc/testsuite/g++.dg/parse/error36.C
@@ -12,7 +12,7 @@ void f(T t)
 {
   typedef A<T>::foo type;	// { dg-error "typename" }
   A<T>::bar b;			// { dg-error "typename" "typename" }
-} // { dg-error "expected ';'" "expected" { target *-*-* } .-1 }
+}
 
 // PR c++/36353
 template <class T> struct B
@@ -20,7 +20,7 @@ template <class T> struct B
   void f()
   {
     A<T>::baz z;		// { dg-error "typename" "typename" }
-  } // { dg-error "expected ';'" "expected" { target *-*-* } .-1 }
+  }
 };
 
 // PR c++/40738
diff --git a/gcc/testsuite/g++.dg/tc1/dr147.C b/gcc/testsuite/g++.dg/tc1/dr147.C
index 6b656491e81..ced18d1879c 100644
--- a/gcc/testsuite/g++.dg/tc1/dr147.C
+++ b/gcc/testsuite/g++.dg/tc1/dr147.C
@@ -21,7 +21,7 @@ void A::f()
 void f()
 {
   A::A a; // { dg-error "constructor" "constructor" }
-} // { dg-error "" "error cascade" { target *-*-* } .-1 } error cascade
+}
 }
 
 namespace N2 {
diff --git a/gcc/testsuite/g++.old-deja/g++.other/typename1.C b/gcc/testsuite/g++.old-deja/g++.other/typename1.C
index 4e1a4a834dd..0f0f68b1cee 100644
--- a/gcc/testsuite/g++.old-deja/g++.other/typename1.C
+++ b/gcc/testsuite/g++.old-deja/g++.other/typename1.C
@@ -14,4 +14,4 @@ template<class T>
 void f()
 {
   Vector<T>::iterator i = 0; // { dg-error "typename" "typename" } missing typename
-} // { dg-error "expected" "expected" { target *-*-* } .-1 }
+}
Jason Merrill Aug. 27, 2024, 6:13 p.m. UTC | #3 
On 8/27/24 1:15 PM, Simon Martin wrote:
> Hi Jason,
> 
> On 26 Aug 2024, at 19:30, Jason Merrill wrote:
> 
>> On 8/26/24 12:49 PM, Simon Martin wrote:
>>> We mention 'X::__ct' instead of 'X::X' in the "names the constructor,
> 
>>> not the type" error for this invalid code:
>>>
>>> === cut here ===
>>> struct X {};
>>> void g () {
>>>     X::X x;
>>> }
>>> === cut here ===
>>>
>>> The problem is that we use %<%T::%D%> to build the error message,
>>> while
>>> %qE does exactly what we need since we have DECL_CONSTRUCTOR_P. This
>>> is
>>> what this patch does, along with skipping until the end of the
>>> statement
>>> to avoid emitting extra (useless) errors.
>>>
>>> Successfully tested on x86_64-pc-linux-gnu.
>>>
>>> 	PR c++/105483
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* parser.cc (cp_parser_expression_statement): Use %qE instead of
>>> 	incorrect %<%T::%D%>, and skip to end of statement.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/tc1/dr147.C: Adjust test expectation.
>>> 	* g++.dg/diagnostic/pr105483.C: New test.
>>>
>>> ---
>>>    gcc/cp/parser.cc                           | 7 ++++---
>>>    gcc/testsuite/g++.dg/diagnostic/pr105483.C | 7 +++++++
>>>    gcc/testsuite/g++.dg/tc1/dr147.C           | 2 +-
>>>    3 files changed, 12 insertions(+), 4 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/diagnostic/pr105483.C
>>>
>>> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
>>> index 28ebf2beb60..ef4e3838a86 100644
>>> --- a/gcc/cp/parser.cc
>>> +++ b/gcc/cp/parser.cc
>>> @@ -13240,10 +13240,11 @@ cp_parser_expression_statement (cp_parser*
>>> parser, tree in_statement_expr)
>>>    	       && DECL_CONSTRUCTOR_P (get_first_fn (statement)))
>>>    	{
>>>    	  /* A::A a; */
>>> -	  tree fn = get_first_fn (statement);
>>>    	  error_at (token->location,
>>> -		    "%<%T::%D%> names the constructor, not the type",
>>> -		    DECL_CONTEXT (fn), DECL_NAME (fn));
>>> +		    "%qE names the constructor, not the type",
>>> +		    get_first_fn (statement));
>>> +	  cp_parser_skip_to_end_of_block_or_statement (parser);
>>
>> Why block_or_statement rather than just _statement?
> It was a mistake, thanks for catching it!
> 
>> Maybe move the skip+return out of this block to share it with the
>> preceding typename error?
> Good idea. It’s a tiny bit more involved than just moving, because
> we’d miss genuine errors emitted by
> cp_parser_consume_semicolon_at_end_of_statement (e.g. break the
> c-c++-common/pr44515.c test, among others), however the updated patch
> 
> does what you’re suggesting. I have successfully tested on
> x86_64-pc-linux-gnu. OK for trunk?

OK.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 28ebf2beb60..ef4e3838a86 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -13240,10 +13240,11 @@  cp_parser_expression_statement (cp_parser* parser, tree in_statement_expr)
 	       && DECL_CONSTRUCTOR_P (get_first_fn (statement)))
 	{
 	  /* A::A a; */
-	  tree fn = get_first_fn (statement);
 	  error_at (token->location,
-		    "%<%T::%D%> names the constructor, not the type",
-		    DECL_CONTEXT (fn), DECL_NAME (fn));
+		    "%qE names the constructor, not the type",
+		    get_first_fn (statement));
+	  cp_parser_skip_to_end_of_block_or_statement (parser);
+	  return error_mark_node;
 	}
     }
 
diff --git a/gcc/testsuite/g++.dg/diagnostic/pr105483.C b/gcc/testsuite/g++.dg/diagnostic/pr105483.C
new file mode 100644
index 00000000000..b935bacea11
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/pr105483.C
@@ -0,0 +1,7 @@ 
+// PR c++/105483
+// { dg-do compile }
+
+struct X { };
+void g () {
+  X::X x; // { dg-error "'X::X' names the constructor" }
+}
diff --git a/gcc/testsuite/g++.dg/tc1/dr147.C b/gcc/testsuite/g++.dg/tc1/dr147.C
index 6b656491e81..ced18d1879c 100644
--- a/gcc/testsuite/g++.dg/tc1/dr147.C
+++ b/gcc/testsuite/g++.dg/tc1/dr147.C
@@ -21,7 +21,7 @@  void A::f()
 void f()
 {
   A::A a; // { dg-error "constructor" "constructor" }
-} // { dg-error "" "error cascade" { target *-*-* } .-1 } error cascade
+}
 }
 
 namespace N2 {