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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
12175 | 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? |
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
12175 | to answer my own question: 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. |
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 | ||
---|---|---|
12429 | -> UD->shadow_size() | |
12459–12460 | Does this comment needs update? | |
12547 | trim off newline | |
12644 | same here. | |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
3083 | here too | |
clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp | ||
20 |
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 | ||
---|---|---|
12429 | Hm, I wonder what happened, I was pretty sure I ran git clang-format. |
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.
Overall looks good, one more comment below.
clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p3.cpp | ||
---|---|---|
20 | How about this one? |
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
12459–12460 | 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? |
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. |
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.
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. |
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?