This is an archive of the discontinued LLVM Phabricator instance.

[clang] p1099 3/5: using Enum::member
ClosedPublic

Authored by urnathan on Apr 11 2021, 3:35 PM.

Details

Summary

This adds support for p1099's 'using SCOPED_ENUM::MEMNER;' functionality, bringing a member of an enumerator into the current scope. The novel feature here, is that there need not be a class hierarchical relationship between the current scope and the scope of the SCOPED_ENUM. That's a new thing, the closest equivalent is a typedef or alias declaration. But this means that Sema::CheckUsingDeclQualifier needs adjustment. (a) one can't call it until one knows the set of decls that are being referenced -- if exactly one is an enumerator, we're in the new territory. Thus it needs calling later in some cases. Also (b) there are two ways we hold the set of such decls. During parsing (or instantiating a dependent scope) we have a lookup result, and during instantiation we have a set of shadow decls. Thus two optional arguments, at most one of which should be non-null.

Diff Detail

Unit TestsFailed

Event Timeline

urnathan requested review of this revision.Apr 11 2021, 3:35 PM
urnathan created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 3:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
urnathan added inline comments.Apr 12 2021, 5:44 AM
clang/lib/Sema/SemaDeclCXX.cpp
12132

I noticed this in passing. I have found it useful to add dates in such workaround comments -- having found really old ones in GCC that have been obsolete for like 15 years or more :) Which version(s) of libstdc++ got this wrong?

urnathan added inline comments.Apr 12 2021, 6:00 AM
clang/lib/Sema/SemaDeclCXX.cpp
12132

to answer my own question:
f501cc313e9e0 (Richard Smith 2012-10-05 01:46:25 +0000

Workaround for libstdc++4.6 <atomic> bug: make comment more explicit about what's going on, per Sean Silva's suggestion.

the last 4.6 release was GCC 4.6.4 in 2013-04-12

so (a) probably removable but I've not checked if/when libstdc++ fixed the problem, and (b) should be treated as a separate issue, of course.

urnathan updated this revision to Diff 337416.Apr 14 2021, 4:59 AM

Remove orthogonal lbstdc++ FIXME comment

bruno added a comment.Apr 20 2021, 4:55 PM

Hi Nathan, thanks for implementing this. Besides formatting nitpicks, few other comments/questions:

  • Update needed in cxx_status.html
  • Should we support this as an extension for earlier C++ versions? This is a very handy feature. In clang terms, warn_cxx17_compat_constexpr_body_invalid_stmt and ext_constexpr_body_invalid_stmt_cxx20 would be examples for that.

Perhaps deferring the scope check until after construction of the shadow-decls in the parsing case would be preferred?

Doing it as part of BuildUsingDeclaration seem reasonable, anything that might not work because of that?

clang/lib/Sema/SemaDeclCXX.cpp
12385

-> UD->shadow_size()

12415–12416

Does this comment needs update?

12503

trim off newline

12600

same here.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
3049

here too

clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp
20

The change to clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp is to silence an uninteresting access error that the above change causes.

The fact that the access check changed for previous version of the language (not necessarily related to p1099) indicates that this specific change deserves its own patch.

clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p7-cxx20.cpp
2

Why not c++17?

thanks for the comments. My plan wrt documentation was to address that (and the feature macro) once the patches are in.

clang/lib/Sema/SemaDeclCXX.cpp
12385

Hm, I wonder what happened, I was pretty sure I ran git clang-format.

urnathan updated this revision to Diff 339293.Apr 21 2021, 10:06 AM

Updated to fix formatting etc (pretty sure I got clang-format to work this time). I changed the pre-c++20 behaviour to be a warning along the lines you suggested. Trying to move the qualifier checking until after generating the shadow decls died horribly -- the shadow generation presumes the context is sane :(

As mentioned, I intend updating docs and feature macro once the second half is done.

urnathan updated this revision to Diff 344396.May 11 2021, 7:34 AM
urnathan edited the summary of this revision. (Show Details)
urnathan edited reviewers, added: davrec; removed: Quuxplusone.

This update relies upon D101777 & D102239 (let's see if I can figure out how to note that elsewhere)

urnathan retitled this revision from [clang] p1099 using enum part 1 to [clang] p1099 3/5: using Enum::member.May 11 2021, 7:49 AM

Overall looks good, one more comment below.

clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp
20

How about this one?

urnathan marked 5 inline comments as done.May 14 2021, 10:39 AM
urnathan added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
12415–12416

I don't think so. 'if we weren't able to compute a scope' is never true for the using-enum case (and neither is 'if we have a typename keyword'. what is it that you're finding confusing here?

urnathan updated this revision to Diff 345487.May 14 2021, 10:39 AM

Address Bruno's comments

urnathan marked an inline comment as done.May 14 2021, 10:42 AM
urnathan added inline comments.
clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp
20

I didn;t describe this very well. What happens with the reordering of the checks of the named scopes is that we now diagnose that C::g is private and then that we're reaching into a struct. Before we just diagnosed the second problem and bailed, never getting to the first. The test is aiming to discover that second problem is detected, and doesn't care about the access. Hence make it public.

urnathan updated this revision to Diff 345959.May 17 2021, 12:07 PM
urnathan edited the summary of this revision. (Show Details)

rebased, trying to resolve patch 4's bot fail

urnathan updated this revision to Diff 345973.May 17 2021, 12:44 PM
urnathan edited the summary of this revision. (Show Details)

good, it appears the patch bot fail is fixed -- it seems unhappy with non-tree-like dependency graphs, and tries to apply each reachable patch as many times as it is reachable :(
The data race failure seems entirely unconnected with this patch -- please correct me if I'm wrong.

bruno accepted this revision.May 21 2021, 1:26 PM

LGTM. An additional review here would be nice though.

good, it appears the patch bot fail is fixed -- it seems unhappy with non-tree-like dependency graphs, and tries to apply each reachable patch as many times as it is reachable :(
The data race failure seems entirely unconnected with this patch -- please correct me if I'm wrong.

Indeed!

clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp
20

Oh I see, makes sense. Thanks for the explanation.

This revision is now accepted and ready to land.May 21 2021, 1:26 PM
davrec accepted this revision.May 22 2021, 8:39 AM
urnathan marked 2 inline comments as done.May 24 2021, 8:27 AM
urnathan added 1 blocking reviewer(s): rsmith.May 25 2021, 10:24 AM
This revision now requires review to proceed.May 25 2021, 10:24 AM
urnathan updated this revision to Diff 349044.Jun 1 2021, 11:52 AM

Rebase and fix conflict

urnathan updated this revision to Diff 349634.Jun 3 2021, 12:02 PM

Rebasing again. Please review!

urnathan removed 1 blocking reviewer(s): rsmith.Jun 7 2021, 5:07 AM
This revision is now accepted and ready to land.Jun 7 2021, 5:07 AM
This revision was automatically updated to reflect the committed changes.