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

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Looks good, a few notes.



================
Comment at: clang/include/clang/AST/ASTContext.h:1559
 
+  QualType getUsingType(const NamedDecl *Found, QualType Underlying) const;
+
----------------
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?


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:984
 
+DEF_TRAVERSE_TYPE(UsingType, { TRY_TO(TraverseType(T->getUnderlyingType())); })
 DEF_TRAVERSE_TYPE(UnresolvedUsingType, {})
----------------
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.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1256
 
+DEF_TRAVERSE_TYPELOC(UsingType,
+                     { TRY_TO(TraverseTypeLoc(TL.getUnderlyingLoc())); })
----------------
DEF_TRAVERSE_TYPELOC(UsingType, {})


================
Comment at: clang/include/clang/AST/Type.h:4381
+public:
+  NamedDecl *getFoundDecl() const { return Found; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }
----------------
 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).



================
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> {
----------------
Should this be analogous to TypedefTypeLoc, with base as
```
 InheritingConcreteTypeLoc<TypeSpecTypeLoc,
                                                  UsingTypeLoc,
                                                  UsingType>
```
?


================
Comment at: clang/include/clang/AST/TypeProperties.td:366
+let Class = UsingType in {
+  def : Property<"foundDeclaration", NamedDeclRef> {
+    let Read = [{ node->getFoundDecl() }];
----------------
Rename to "declaration" iff renaming UsingType::getFoundDecl to UsingType::getDecl


================
Comment at: clang/lib/AST/ASTContext.cpp:4612
+    Canon = getCanonicalType(Underlying);
+  UsingType *NewType = new UsingType(Found, Underlying, Canon);
+  Types.push_back(NewType);
----------------
UsingType *NewType = new (*this, TypeAlignment) UsingType(Found, Underlying, Canon)


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:952
+      return false;
+    if (!IsStructurallyEquivalent(Context,
+                                  cast<UsingType>(T1)->getUnderlyingType(),
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114251