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

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.
This revision is now accepted and ready to land.

Looks great, thanks for identifying the need and banging this out so quickly.
Hope you had some time to enjoy the holiday with your family!
Dave



================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2106
+        TRY_TO(TraverseDecl(Child));
+    }
   }
----------------
So the idea is, the UsingDecl which introduced the shadows will have already been traversed within its DeclStmt via the body traversal, but not the UsingShadowDecls it introduced?  Nicely spotted.


================
Comment at: clang/include/clang/AST/Type.h:4381
+public:
+  NamedDecl *getFoundDecl() const { return Found; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }
----------------
sammccall wrote:
> 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.
I'm persuaded by your reasoning, particularly that UsingShadowDecl is invisible and thus returning one already introduces some confusion -- i.e. a user might reasonably expect getDecl() to return a UsingDecl, so better to call this something with other parallels.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114251