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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
