On 2/10/25 12:09 PM, Simon Martin wrote:
Hi Jason,
On 7 Feb 2025, at 23:10, Jason Merrill wrote:
On 2/7/25 4:04 PM, Simon Martin wrote:
Hi Jason,
On 7 Feb 2025, at 14:21, Jason Merrill wrote:
On 2/6/25 3:05 PM, Simon Martin wrote:
Hi Jason,
On 6 Feb 2025, at 16:48, Jason Merrill wrote:
On 2/5/25 2:21 PM, Simon Martin wrote:
Hi Jason,
On 4 Feb 2025, at 21:23, Jason Merrill wrote:
On 2/4/25 3:03 PM, Jason Merrill wrote:
On 2/4/25 11:45 AM, Simon Martin wrote:
On 4 Feb 2025, at 17:17, Jason Merrill wrote:
On 2/4/25 10:56 AM, Simon Martin wrote:
Hi Jason,
On 4 Feb 2025, at 16:39, Jason Merrill wrote:
On 1/15/25 9:56 AM, Jason Merrill wrote:
On 1/15/25 7:24 AM, Simon Martin wrote:
Hi,
On 14 Jan 2025, at 23:31, Jason Merrill wrote:
On 1/14/25 2:13 PM, Simon Martin wrote:
On 10 Jan 2025, at 19:10, Andrew Pinski wrote:
On Fri, Jan 10, 2025 at 3:18 AM Simon Martin
<si...@nasilyan.com>
wrote:
We currently accept the following invalid code (EDG
and
MSVC
do
as
well)
clang does too:
https://github.com/llvm/llvm-project/issues/121706 .
Note it might be useful if a testcase with multiply
`*`
is
included
too:
```
struct A {
****A ();
};
```
Thanks, makes sense to add those. Done in the attached
updated
revision,
successfully tested on x86_64-pc-linux-gnu.
+/* Check that it's OK to declare a function at ID_LOC
with
the
indicated TYPE,
+ TYPE_QUALS and DECLARATOR. SFK indicates the
kind
of
special
function (if
+ any) that this function is. OPTYPE is the type
given
in
a
conversion
operator declaration, or the class type
for
a
constructor/destructor.
Returns the actual return type of the
function;
that
may
be
different
than TYPE if an error occurs, or for
certain
special
functions.
*/
@@ -12361,8 +12362,19 @@
check_special_function_return_type
(special_function_kind sfk,
tree
type,
tree
optype,
int
type_quals,
+ const
cp_declarator
*declarator,
+ location_t
id_loc,
id_loc should be the same as declarator->id_loc?
You’re right.
const
location_t*
locations)
{
+ /* If TYPE is unspecified, DECLARATOR, if set,
should
not
represent a pointer
+ or a reference type. */
+ if (type == NULL_TREE
+ && declarator
+ && (declarator->kind == cdk_pointer
+ || declarator->kind == cdk_reference))
+ error_at (id_loc, "expected unqualified-id
before
%qs
token",
+ declarator->kind == cdk_pointer ?
"*"
:
"&");
...and id_loc isn't the location of the ptr-operator,
it's
the
location of the identifier, so this indicates the wrong
column.
I
think using declarator->id_loc makes sense, just not
pretending
it's
the location of the *.
Good catch, thanks.
Let's give diagnostics more like the others later in the
function
instead of trying to emulate cp_parser_error.
Makes sense. This is what the updated patch does,
successfully
tested on
x86_64-pc-linux-gnu. OK for GCC 16?
OK.
Does this also fix 118304? If so, let's go ahead and
apply
it
to
GCC
15.
I have checked just now, and we still ICE for 118304’s
testcase
with
that fix.
Why doesn't the preeexisting
type = void_type_node;
in check_special_function_return_type fix the return type and
avoid
the ICE?
We hit the gcc_assert at method.cc:3593, that Marek’s fix
bypasses.
Yes, but why doesn't check_special_function_return_type prevent
that?
Ah, because we call it before walking the declarator. We need
to
check again later, perhaps in grokfndecl, that the type is
correct.
Perhaps instead of your patch.
One “issue” with adding another check in or close to
grokfndecl
is
that DECLARATOR will have “been moved to the ID”, and the
fact
that
we had a CDK_POINTER kind is “lost”. We could obviously
somehow
propagate this information, but there might be something easier.
The information isn't lost: it's now reflected in the (wrong)
return
type. One place it would make sense to check would be
if (ctype && (sfk == sfk_constructor
|| sfk == sfk_destructor))
{
/* We are within a class's scope. If our
declarator
name
is the same as the class name, and we are
defining
a
function, then it is a constructor/destructor, and
therefore
returns a void type. */
Here 'type' is still the return type, we haven't gotten to
build_function_type yet.
That’s true. However, doesn’t it make sense to cram all the
checks
about the return type of special functions in
check_special_function_return_type, and return an error if that
return
type is invalid?
This error seems easily recoverable since we know what the type
needs
to be, there's no need for error return from grokdeclarator.
ACK.
However, an alternative to my suggestion above would be to build on
your patch by making check_special_function_return_type actually
strip
the invalid declarators, not just complain about them.
Thanks for the suggestion, I like that it keeps everything within
check_special_function_return_type. Hence the updated attached patch,
successfully tested on 86_64-pc-linux-gnu. OK for trunk?
case sfk_constructor:
if (type)
error_at (smallest_type_location (type_quals, locations),
"return type specification for constructor invalid");
+ else if (maybe_strip_indirect_ref (declarator))
+ error_at ((*declarator)->id_loc,
+ "return type specification for constructor invalid");
else if (type_quals != TYPE_UNQUALIFIED)
error_at (smallest_type_quals_location (type_quals, locations),
"qualifiers are not allowed on constructor declaration");
This looks like if someone writes e.g.
int *A();
then we'll complain about the 'int' but leave the * alone.
Argh you’re right, the return type is not fully groked at this
point, so check_special_function_return_type should check first
for indirect refs and then for type if needed. This is what the
updated attached patch does
Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
OK, thanks.
Jason
