[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 7 inline comments as done.
sammccall added a comment.

Thanks a lot for the comments, very helpful.
The next revision is much narrower: it'll only sugar exactly the type associated with a UsingShadowDecl. It no longer wraps the underlying type.
I've added fixes for all the clang-tools-extra breakages, and enough AST-matchers to support that.

(Slow turnaround: I am interested in this but quarantined with small kids right now...)



================
Comment at: clang/include/clang/AST/ASTContext.h:1559
 
+  QualType getUsingType(const NamedDecl *Found, QualType Underlying) const;
+
----------------
davrec wrote:
> I believe you mentioned ObjC considerations might require expanding beyond UsingShadowDecl as the type of `Found` -- is that why this is a generic NamedDecl?  I.e. can `Found` indeed be other things than a UsingShadowDecl?
That was the reason. NamedDecl is what lookup returns, and the code around DeclRefExpr suggests it can be an objc compatibility alias.

However I'm not sure trying to generalize across cases I don't understand well is a great idea. I've narrowed this to cover UsingShadowDecl only.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:984
 
+DEF_TRAVERSE_TYPE(UsingType, { TRY_TO(TraverseType(T->getUnderlyingType())); })
 DEF_TRAVERSE_TYPE(UnresolvedUsingType, {})
----------------
davrec wrote:
> This should just be DEF_TRAVERSE_TYPE(UsingType, {}), i.e. same as for TypedefType -- RecursiveASTVisitor only visits the child nodes, to avoid retraversal of any nodes; by traversing the desugared node this would jump back to a node it probably already traversed.
You're right, changed the underlying type to not be a child.

(Before I'd fully thought through the template case, I thought we wanted to UsingType to wrap TemplateSpecializationType, and so it had to contain the underlying type to capture the args. But that model is silly, and templates are best handled in TemplateName separately)


================
Comment at: clang/include/clang/AST/Type.h:4381
+public:
+  NamedDecl *getFoundDecl() const { return Found; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }
----------------
davrec wrote:
>  I would rename this to `getDecl()`, to match the interface for other types that just wrap a decl.  E.g. if something is a RecordType I know I can call getDecl() to get the RecordDecl; likewise a TypedefType::getDecl() returns a TypedefNameDecl; I think it would follow this pattern for UsingType to have a getDecl() method that returns a UsingShadowDecl (or whatever else it can be, per other question).
> 
I do prefer `getFoundDecl()` for a few reasons:
 - the parallel with `NamedDecl::getFoundDecl()` is closer/more important than with `TypedefDecl` I think
 - there are always two decls here: the invisible UsingShadowDecl and the underlying one. Saying "decl" without a hint seems error-prone to me. (Compare with TypedefType where in general there's no underlying decl).
 - I **do** find TypedefType::getDecl() confusing, because wherever I see it called I have to verify that it's TypedefType::getDecl() rather than some Type::getDecl() to be sure I understand the semantics.

Would be happy to hear a third opinion here though.


================
Comment at: clang/include/clang/AST/TypeLoc.h:671
+/// Wrapper for source info for types used via transparent aliases.
+class UsingTypeLoc : public ConcreteTypeLoc<UnqualTypeLoc, UsingTypeLoc,
+                                            UsingType, UsingLocInfo> {
----------------
davrec wrote:
> Should this be analogous to TypedefTypeLoc, with base as
> ```
>  InheritingConcreteTypeLoc<TypeSpecTypeLoc,
>                                                   UsingTypeLoc,
>                                                   UsingType>
> ```
> ?
Yup, after switching to not wrapping the underlying type this is much cleaner, thanks!


================
Comment at: clang/lib/AST/ASTContext.cpp:4612
+    Canon = getCanonicalType(Underlying);
+  UsingType *NewType = new UsingType(Found, Underlying, Canon);
+  Types.push_back(NewType);
----------------
davrec wrote:
> UsingType *NewType = new (*this, TypeAlignment) UsingType(Found, Underlying, Canon)
Wow, well spotted, thank you!


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:952
+      return false;
+    if (!IsStructurallyEquivalent(Context,
+                                  cast<UsingType>(T1)->getUnderlyingType(),
----------------
davrec wrote:
> Is there a good reason this checks the underlying type in addition to getFoundDecl()? I'm looking at the Typedef case below and it only checks the decl, does this need to be different from that?
I figured if I was storing both, not checking both would invite trouble.
And in fact I eventually found such a case: where the underlying was a CTAD type and the decl was the template name.

But now I've made sure we don't handle that case, store only the decl, and assert that the underlying type is what we expect.


================
Comment at: clang/lib/AST/Type.cpp:1819
 
+    Type *VisitUsingType(const UsingType *T) {
+      return Visit(T->getUnderlyingType());
----------------
davrec wrote:
> Do we need this here?  I ask because I mainly because don't see a VisitTypedefType, not 100% sure what this class does though.
No longer needed now the underlying cannot be a CTAD type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114251/new/

https://reviews.llvm.org/D114251