[cfe-dev] Advice on patch

[Nathan Sidwell via cfe-dev]

On 4/30/21 12:55 PM, David Rector wrote:
> 
> 
>> On Apr 30, 2021, at 7:14 AM, Nathan Sidwell via cfe-dev 
>> <cfe-dev at lists.llvm.org <mailto: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 
>> <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 <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.")

hehe, a vote for each direction -- my internal dilemma is externalized!
> 
> 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).

Not quite.  Consider
namespace Foo {
   struct bob;
   int bob;
}
using Foo::bob;  // both struct tag and int var now visible!

> 
> So, a structure like this might be clearer:
> ```
> /// A base of both UsingDecl and UsingEnumDecl
> class ShadowIntroducingDecl : public NamedDecl {

That's probably a good name -- put 'Shadow' in all the closely related tags.

>    // 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.

good, agreed.

> 
>> 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.
> 
> 
> 


-- 
Nathan Sidwell