[cfe-dev] Advice on patch

[David Rector via cfe-dev]

> On Apr 30, 2021, at 7:14 AM, Nathan Sidwell via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> hi,  I've been working on p1099 (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html)-- c++20's using-enum feature.  Among other things it allows you to being the enumerators of a scoped enumeration into the local scope 'using enum maybe-qualified-tag-name ;'  Brings the members of that enum in, as if by individual using declarations.  I posted an initial patch seeking advice on a couple of issues as https://reviews.llvm.org/D101370
> 
> I chose to extend EnumDecl, rather than a new decl as we still need the shadow decl chain.  The enum is parsed with ParseEnumSpecifier.
> 
> Q1)  I need to add the EnumDecl to the UsingDecl, and don't need the QualifiedLoc or DNLoc fields.  Should I use a local union to overlay those pieces of data, or just live with the size expansion?
> 

My preference would be to have a separate UsingEnumDecl, and use the opportunity to factor out a common base to handle the shadow stuff, in a manner that would also provide a clearer interface for novice users. (`shadows()` etc. is a bit to abstruse for someone who just wants to quickly get the thing or things being "used.")  

IIUC a UsingDecl presently only ever has more than one shadow when it refers to an overloaded function.  (And now, when it represents a using-enum-declaration with multiple enumerators).

So, a structure like this might be clearer:
 
```
/// A base of both UsingDecl and UsingEnumDecl
class ShadowIntroducingDecl : public NamedDecl {
  // All the shadow stuff from UsingDecl, e.g.:
  llvm::PointerIntPair<UsingShadowDecl *, 1, bool> FirstUsingShadow;
public:
  shadow_range shadows() const;
  unsigned shadow_size() const;
  //...
};

class UsingDecl : public ShadowIntroducingDecl, public Mergeable<UsingDecl> {
  // ...
  // DNLoc, QualifierLoc, anything else specific to UsingDecls.
public:
  // --- Non-essential methods provided for clarity --- //
  bool TargetIsFunction() const { 
    return isa<FunctionDecl>(FirstUsingShadow.getPointer()->getTargetDecl());
  }
  NamedDecl *getTargetAsSingleDecl() { 
    assert(!TargetIsFunction() && "use getTargetOverloads() instead"); 
    assert(shadow_size() == 1);
    return FirstUsingShadow.getPointer()->getTargetDecl(); 
  }

  /// Same as shadow_iterator, but dereferences directly to the target FunctionDecls
  struct function_overload_iterator : shadow_iterator {
    FunctionDecl *operator*() const { return cast<FunctionDecl>(shadow_iterator::operator*()->getTargetDecl()); }
    FunctionDecl *operator->() const //""
    //...
  };
  using function_overload_range = llvm::iterator_range<function_overload_iterator>;

  function_overload_range getTargetOverloads() const { 
    assert(TargetIsFunction()); 
    return static_cast<function_overload_range>(shadows()); //or whatever
  }
  unsigned getNumOverloads() const {
    assert(TargetIsFunction());
    return shadow_size();
  }
};

class UsingEnumDecl : public ShadowIntroducingDecl, public Mergeable<UsingEnumDecl> {
  //...
public:
  EnumDecl *getNominatedEnum() const { 
    return cast<EnumDecl>(FirstShadowDecl.getPointer()->getTargetDecl()->getDeclContext()); 
  }
  auto getTargetEnumerators() const { 
    return getNominatedEnum()->decls();
  }
};

```

> Q2) When the scoped enum is a template member, the members are instantiated when one looks inside the enum -- not when the enum decl is instantiated.  That's done with RequireCompleteDeclContext, which is designed for when the parser is looking, and therefore takes a ScopeRef.  This is a new situation where we don't have a ScopeRef, we have an EnumDecl, which we desire to complete.  Is it best to break RequireCompleteDeclContext apart in some way so there's a new entry point for the new use?
> 

To me it seems reasonable to separate out a `RequireCompleteEnumDecl(EnumDecl *D, CXXScopeSpec *SS = nullptr)` function that handles just the enum-specific stuff in there.

> thanks,
> 
> nathan
> 
> -- 
> Nathan Sidwell
> 

One last thing while we’re on the general topic of UsingDecl, just in case you or anyone else wants to take this opportunity to clean it up more generally.  There is presently no `UsingType` to provide the type sugar that a particular type was introduce via a UsingDecl.  E.g.

```
struct A {
  struct Inner {};
};
struct B : A {
  using A::Inner;
  Inner i;
};

The type dump of `B::i` is

RecordType 0x7fe32c80c2c0 'struct A::Inner'
`-CXXRecord 0x7fe32c80c210 'Inner'

when I think it really should be something like

UsingType *********** 'struct A::Inner' sugar
`-RecordType 0x7fe32c80c2c0 'struct A::Inner'
  `-CXXRecord 0x7fe32c80c210 'Inner'

I think this is may be the only case in the AST for which full source information about a type is not provided via sugar.  And, a quick test suggests the UsingShadowDecl info is available during Lookup (i.e. we know if a particular lookup result is a UsingShadowDecl), so a UsingType would be straightforward to add.



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210430/988216b0/attachment.html>