sammccall added a comment. In D119363#3323867 <https://reviews.llvm.org/D119363#3323867>, @dgoldman wrote:
> In D119363#3323778 <https://reviews.llvm.org/D119363#3323778>, @sammccall > wrote: > >> I'm a bit concerned about the parent map vs ASTMatchFinder's view being out >> of sync: we'll have a ProtocolLoc node that has a parent but the parent >> doesn't have the child. >> >> I'm not sure this breaks anything immediately, but I think we should also >> make these nodes visible to matchers, even if there's no specific matcher >> yet. > > Hmm I can try to do it - what do I need to modify to make them visible to > matchers? I don't remember specifically, I think ASTMatchFinderImpl has a RecursiveASTVisitor that you need to extend? I'm not sure actually how you would observe these nodes cleanly without matchers (hacks like `has(hasParent())` maybe?) So maybe it's best to ignore it in this patch and add basic matchers in a next one ================ Comment at: clang/include/clang/AST/TypeLoc.h:2611 +class ObjCProtocolLoc { + const ObjCProtocolDecl *Protocol = nullptr; + SourceLocation Loc = SourceLocation(); ---------------- dgoldman wrote: > sammccall wrote: > > Also add a public `getLocation()` accessor for ProtocolLoc? (That returns > > SourceLocation) > this was already there? did you want something else? Nope sorry I'm just going blind it seems ================ Comment at: clang/include/clang/AST/TypeLoc.h:2617 + : Protocol(protocol), Loc(loc) {} + const ObjCProtocolDecl *getProtocol() const { return Protocol; } + SourceLocation getLocation() const { return Loc; } ---------------- dgoldman wrote: > sammccall wrote: > > Return non-const pointer, matching other AST nodes. (But method should be > > const) > Why - Is there a reason to make it mutable? TypeLoc's getTypePtr is const > (although this is a decl not a type). A bunch of stuff operates on non-const nodes only, RecursiveASTVisitor for example. And for whatever reason this is the convention, rather than a const+non-const variant of everything. > TypeLoc's getTypePtr is const (although this is a decl not a type). Yeah types are always const, this is related to the fact that they're interned so mutating them wouldn't make sense. ================ Comment at: clang/lib/AST/ParentMapContext.cpp:404 + template<typename T> bool isNull(T t) { return !t; } + template<> bool isNull(ObjCProtocolLoc t) { return false; } ---------------- t -> Node (case) ================ Comment at: clang/lib/AST/ParentMapContext.cpp:404 + template<typename T> bool isNull(T t) { return !t; } + template<> bool isNull(ObjCProtocolLoc t) { return false; } ---------------- sammccall wrote: > t -> Node (case) These should be `static` I think so they're not exposed ================ Comment at: clang/lib/AST/ParentMapContext.cpp:405 + template<typename T> bool isNull(T t) { return !t; } + template<> bool isNull(ObjCProtocolLoc t) { return false; } + ---------------- No need for template<>, this can just be a normal function that forms an overload set along with the template Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119363/new/ https://reviews.llvm.org/D119363 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
