Page MenuHomePhabricator

[clang] p1099 using enum part 1
Needs ReviewPublic

Authored by urnathan on Sun, Apr 11, 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.

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.

Perhaps deferring the scope check until after construction of the
shadow-decls in the parsing case would be preferred? Also moving up
the check for naming a scoped-enumerator member might be more helpful?

Diff Detail

Event Timeline

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

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.Mon, Apr 12, 6:00 AM
clang/lib/Sema/SemaDeclCXX.cpp
12144

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.Wed, Apr 14, 4:59 AM

Remove orthogonal lbstdc++ FIXME comment

bruno added a comment.Tue, Apr 20, 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
12353

-> UD->shadow_size()

12370

Does this comment needs update?

12452

trim off newline

12538

same here.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
3079

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
12353

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

urnathan updated this revision to Diff 339293.Wed, Apr 21, 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.