This is an archive of the discontinued LLVM Phabricator instance.

[clang] p1099 4/5: using enum EnumTag
ClosedPublic

Authored by urnathan on May 11 2021, 7:43 AM.

Details

Summary

This relies upon D101777 & D102239
This implements the 'using enum maybe-qualified-enum-tag ;' part of 1099. It introduces a new 'UsingEnumDecl', subclassed from 'BaseUsingDecl'. Much of the diff is the boilerplate needed to get the new class set up.

There is one case where we accept ill-formed, but I believe this is merely an extended case of an existing bug, so consider it orthogonal. AFAICT in class-scope the c++20 rule is that no 2 using decls can bring in the same target decl ([namespace.udecl]/8). But we already accept:

struct A { enum { a }; };
struct B : A { using A::a; };
struct C : B { using A::a;

using B::a; }; // same enumerator

this patch permits mixtures of 'using enum Bob;' and 'using Bob::member;' in the same way.

Diff Detail

Event Timeline

urnathan created this revision.May 11 2021, 7:43 AM
urnathan requested review of this revision.May 11 2021, 7:43 AM
urnathan retitled this revision from [clang] p1099 using-enum part 2 to [clang] p1099 4/5: using enum EnumTag.May 11 2021, 7:50 AM
bruno added a subscriber: bruno.May 13 2021, 11:23 PM
bruno added inline comments.
clang/include/clang/AST/DeclCXX.h
3577

Since you are adding a new AST node, please add AST dump tests for it, you can looks at examples in clang/test/AST/ast-dump-*.

urnathan updated this revision to Diff 345491.May 14 2021, 10:45 AM

reupload diff

Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 10:45 AM
urnathan updated this revision to Diff 345500.May 14 2021, 10:53 AM

Let's try again. Logically this is separate from patch 3, but because that and this touch adjacent lines in the diagnostics file, we need to say patch 3 is a dependency, otherwise we get a conflict.

I don't understand why the build bot is failing to apply the precursor D101777 diff. that diff is marked as ok by the build bot. How can I find the actual error the build bot is encountering?

urnathan updated this revision to Diff 345848.May 17 2021, 5:52 AM

Add serialization test and fix bug so found (thanks Bruno!)

urnathan marked an inline comment as done.May 17 2021, 5:53 AM
urnathan updated this revision to Diff 345960.May 17 2021, 12:08 PM

rebased, trying to resolve bot failure, again

martong added inline comments.May 18 2021, 2:08 AM
clang/lib/AST/ASTImporter.cpp
4656

Could it work if you imported the shadow declarations before creating the UsingEnumDecl? I yes, then that would be the preferred approach b/c that would fix the // FIXME: We return error here but the definition is already created.

urnathan updated this revision to Diff 346107.May 18 2021, 3:56 AM

updating to reset dependency graph and coax build bot into working

urnathan added inline comments.May 18 2021, 3:58 AM
clang/lib/AST/ASTImporter.cpp
4656

Unfortunately the shadow decls refer back to the using decl, so we must do it in this order. This is the same comment as in the existing UsingDecl importer, not a new fixme unique to using-enum.

urnathan updated this revision to Diff 346421.May 19 2021, 5:53 AM

Added ASTmatchers for UsingEnumDecl along with unit-tests (both of which I was previously unaware of)

bruno accepted this revision.May 20 2021, 6:12 PM
bruno added a reviewer: rsmith.

Thanks for adding the tests. LGTM after some remaining nitpick.

clang/lib/Sema/SemaDeclCXX.cpp
11640

Remove this empty line.

12346

No need for curly braces here.

This revision is now accepted and ready to land.May 20 2021, 6:12 PM
urnathan marked an inline comment as done.May 21 2021, 4:25 AM
urnathan added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
11640

You guys sure hate white space :) FWIW, here's the using decl' source:

assert(IdentLoc.isValid() && "Invalid TargetName location.");

// FIXME: We ignore attributes for now.

// For an inheriting constructor declaration, the name of the using
urnathan marked an inline comment as done.May 21 2021, 4:26 AM
davrec added a subscriber: davrec.May 22 2021, 10:45 AM
davrec added inline comments.
clang/include/clang/AST/ASTContext.h
520–523

We need a detailed example in the documentation, since IIUC P1099 does not allow a using-enum-declaration to name "dependent" enums and thus is distinguished from using-declarations. Specifically the wording is:

using-enum-declaration:

using elaborated-enum-specifier ;
  1. The elaborated-enum-specifier shall not name a dependent type and the type shall have a reachable enum-specifier.

...

(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html)

Now I'm not 100% clear on what that wording permits, but we need an example of how a UsingEnumDecl can be an instantiation. Something like this maybe?

template<unsigned N>
struct Foo {
  enum E { e = N };
};
template<unsigned N>
struct Bar : Foo<N> {
  using enum Foo<N>::E; //Allowed per P1099?
};
Bar<1>;

We can also clarify the types here to

llvm::DenseMap<UsingEnumDecl *, UsingEnumDecl *>

since there are no UnresolvedUsing*Decl` versions to account for, as there were with using decls.

910–911
/// If the given using decl \p Inst is an instantiation of
/// another (possibly unresolved) using decl, return it.
919–922
/// If the given using-enum decl \p Inst is an instantiation of
/// another using-enum decl, return it.
UsingEnumDecl *getInstantiatedFromUsingEnumDecl(UsingEnumDecl *Inst);
926

UsingEnumDecl *Inst, UsingEnumDecl *Pattern

clang/lib/AST/ASTContext.cpp
1574

NamedDecl -> UsingEnumDecl

1582–1583

NamedDecl -> UsingEnumDecl

davrec added inline comments.May 22 2021, 12:46 PM
clang/include/clang/AST/ASTContext.h
520–523

It occurs to me that, duh, non-dependent declarations in a template still need to be instantiated.
So in summary I would just change these lines to this:

/// Like InstantiatedFromUsingDecl, but for using-enum-declarations. Maps
/// from the instantiated using-enum to the templated decl from whence it
/// came.
/// Note that using-enum-declarations cannot be dependent and
/// thus will never be instantiated from an "unresolved"
/// version thereof (as with using-declarations), so each mapping is from
/// a (resolved) UsingEnumDecl to a (resolved) UsingEnumDecl.
llvm::DenseMap<UsingEnumDecl *, UsingEnumDecl *> InstantiatedFromUsingEnumDecl;
urnathan updated this revision to Diff 347405.May 24 2021, 8:29 AM

Address davrec's comments

urnathan updated this revision to Diff 347406.May 24 2021, 8:33 AM
urnathan marked 8 inline comments as done.
davrec accepted this revision.May 24 2021, 4:57 PM
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 349045.Jun 1 2021, 11:52 AM

rebase and fix conflict

urnathan updated this revision to Diff 349635.Jun 3 2021, 12:03 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 landed with ongoing or failed builds.Jun 8 2021, 11:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 11:12 AM