This is an archive of the discontinued LLVM Phabricator instance.

Patch for Bug 43966 - Further implement of P1131R2: Core Issue 2292
ClosedPublic

Authored by Manna on Jan 4 2020, 9:00 PM.

Details

Summary

The patch adds support for further implementation of P1131R2: Core Issue 2292 and fixes PR43966.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=43966

The core issue is that simple-template-id is ambiguous between class-name and type-name.

Previous Commit Review:
https://reviews.llvm.org/rGee0f1f1edc3ec0d4e698d50cc3180217448802b7

The patch got reverted due to buildbot failure in LLVM and new error in chromium code.

Diff Detail

Event Timeline

Manna created this revision.Jan 4 2020, 9:00 PM
aaron.ballman added inline comments.Jan 6 2020, 10:34 AM
clang/lib/Sema/SemaExprCXX.cpp
196–200

} else if (auto *RD = dyn_cast_or_null<CXXRecordDecl>(DC)) {

198

You can then drop the RD test here (which wasn't useful anyway given that we dereference RD before checking if it's null). I'm not certain what this was trying to test though -- it looks like we want (RD->hasDefinition() && RD->hasSimpleDestructor()) || !RD->hasDefinition()?

Manna updated this revision to Diff 237722.Jan 13 2020, 10:17 AM
Manna marked 2 inline comments as done.Jan 13 2020, 10:25 AM
Manna added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
198

Thank you @aaron.ballman for looking into the patch and the feedback. Yes, RD is not needed here. I have fixed this.

Manna updated this revision to Diff 237726.Jan 13 2020, 10:36 AM
Manna marked 2 inline comments as done.

Patch is updated based on the comments. Thanks.

aaron.ballman accepted this revision.Jan 14 2020, 6:45 AM

The changes LGTM, but I notice that our DR status page says we already supported CWG2292 as of Clang 9 (Richard changed the status in svn:368941) which surprises me. Was this aspect just missed with the original commit, or is something else going on?

This revision is now accepted and ready to land.Jan 14 2020, 6:45 AM

The changes LGTM, but I notice that our DR status page says we already supported CWG2292 as of Clang 9 (Richard changed the status in svn:368941) which surprises me. Was this aspect just missed with the original commit, or is something else going on?

Thanks @aaron.ballman for accepting the patch. The test is failing to compile with latest clang 10. I believe this aspect was just missed with the original commit. This example is valid in P1131R2: Core Issue 2292: simple-template-id is ambiguous between class-name and type-name
-bash-4.2$ clang -v
clang version 10.0.0

If you are OK with it, could you please commit the patch?

Thanks,
Soumi Manna

aaron.ballman closed this revision.Jan 15 2020, 5:50 AM

Thank you for the patch, I've committed on your behalf in ee0f1f1edc3ec0d4e698d50cc3180217448802b7

Manna added a comment.Jan 15 2020, 8:40 AM

Thank you for the patch, I've committed on your behalf in ee0f1f1edc3ec0d4e698d50cc3180217448802b7

Thank you so much. I greatly appreciate this.

Manna updated this revision to Diff 243229.Feb 7 2020, 10:42 AM
Manna retitled this revision from Patch for Bug 43966 - Clang compiler fails with error: expected the class name after '~' to name a destructor to Patch for Bug 43966 - Further implement of P1131R2: Core Issue 2292.
Manna edited the summary of this revision. (Show Details)
Manna added a reviewer: akhuang.

Hi,
I think the second test case is incorrect. The wording from P1131r2 is:

"If a typedef-name is used to identify the subject of an elaborated-type-specifier (9.1.7.3), a class definition (Clause 10 [class]), a constructor declaration (10.3.4 [class.ctor]), or a destructor declaration (10.3.7 [class.dtor]), the program is ill-formed. -- end note ]"

The phrase "if a typedef-name is used to identify the subject of" seems to apply only to the unqualified-id after the nested-name-specifier. The unqualified-id in your test case is 'B', which is not a typedef when looked up in the type referred to by the nested-name-specifier. Interpreting this passage to disqualify a typedef name as a component of the nested-name-specifier would be inconsistent with other areas of the Standard that allow alias templates and other typedefs as components of the nested-name-specifier. Furthermore, if you remove the '~' in your test case (and add a constructor declaration to the class B), the test will compile without error as you still allow the equivalent use of a type alias to declare the *constructor*. The excerpt from P113r2 included above does not distinguish between constructor and destructor, so they should either both be ill-formed or neither.