Page MenuHomePhabricator

[clang]: Remove assertion which checks explicit declaration
ClosedPublic

Authored by gousemoodhin on Jul 15 2020, 9:57 PM.

Details

Summary

explicit keyword is declared outside of class is invalid, invalid explicit declaration is handled inside DiagnoseFunctionSpecifiers() function. To avoid compiler crash in case of invalid explicit declaration, remove assertion.

Diff Detail

Event Timeline

gousemoodhin created this revision.Jul 15 2020, 9:57 PM
gousemoodhin added a reviewer: Restricted Project.Jul 22 2020, 12:46 PM
arsenm added a subscriber: arsenm.Jul 29 2020, 6:17 PM
arsenm added inline comments.
clang/test/Misc/explicit.c
1 ↗(On Diff #279908)

This is missing a RUN line and thus doesn't test anything

rsmith requested changes to this revision.Jul 30 2020, 12:40 PM
rsmith added a subscriber: rsmith.

We already check that explicit is only used in the proper scopes in ActOnFunctionDeclarator (and the proper scopes aren't just class scopes, due to deduction guides). In fact, the assert appears to have nothing to do with whether the expression appears at function scope; that's a red herring caused by unary && (address of label) doing different things at that scope. An analogous case crashes at class scope too:

void *p;
struct X { explicit(p) X(); };

It looks like this is just a bogus assertion; when ActOnExplicitBoolSpecifier produces an invalid ExplicitSpecifier and the parser stores it on the DeclSpec, the assertion that's triggering is in effect asserting that the ExplicitSpecifier is not invalid, which seems wrong, because it certainly can be invalid. We'll need to check that later users of the ExplicitSpecifier can cope with it being invalid, but subject to that, removing the assertion seems like the right change to me.

This revision now requires changes to proceed.Jul 30 2020, 12:40 PM
gousemoodhin added a comment.EditedJul 31 2020, 10:11 AM

We already check that explicit is only used in the proper scopes in ActOnFunctionDeclarator (and the proper scopes aren't just class scopes, due to deduction guides). In fact, the assert appears to have nothing to do with whether the expression appears at function scope; that's a red herring caused by unary && (address of label) doing different things at that scope. An analogous case crashes at class scope too:

void *p;
struct X { explicit(p) X(); };

It looks like this is just a bogus assertion; when ActOnExplicitBoolSpecifier produces an invalid ExplicitSpecifier and the parser stores it on the DeclSpec, the assertion that's triggering is in effect asserting that the ExplicitSpecifier is not invalid, which seems wrong, because it certainly can be invalid. We'll need to check that later users of the ExplicitSpecifier can cope with it being invalid, but subject to that, removing the assertion seems like the right change to me.

Yes, I agree, declaration of explicit is varified inside ActOnFunctionDeclarator(), removing the assertion looks correct.

gousemoodhin retitled this revision from [clang]: Check the explicit keyword declaration to [clang]: Remove assertion which checks explicit declaration.
gousemoodhin edited the summary of this revision. (Show Details)
jkorous removed a reviewer: Restricted Project.Aug 4 2020, 1:06 PM

Please add a corresponding test case. Other than that this looks good. Thanks!

gousemoodhin edited the summary of this revision. (Show Details)

Please add a corresponding test case. Other than that this looks good. Thanks!

Done.

rsmith accepted this revision.Aug 20 2020, 3:38 PM

Looks good to me. Do you need someone to land this for you?

This revision is now accepted and ready to land.Aug 20 2020, 3:38 PM

Please add a corresponding test case. Other than that this looks good. Thanks!

Done.

Looks good to me. Do you need someone to land this for you?

Yes,
Please use these details to commit.
My Github username: gousemoodhin
My GitHub email id: nadafgouse5@gmail.com

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2020, 6:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript