sammccall added a comment.
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.
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1350
+DEF_TRAVERSE_TYPELOC(ObjCTypeParamType, {
+ for (unsigned i = 0, n = TL.getNumProtocols(); i != n; ++i) {
+ ObjCProtocolLoc ProtocolLoc(TL.getProtocol(i), TL.getProtocolLoc(i));
----------------
i => I etc
================
Comment at: clang/include/clang/AST/TypeLoc.h:2611
+class ObjCProtocolLoc {
+ const ObjCProtocolDecl *Protocol = nullptr;
+ SourceLocation Loc = SourceLocation();
----------------
Also add a public `getLocation()` accessor for ProtocolLoc? (That returns
SourceLocation)
================
Comment at: clang/include/clang/AST/TypeLoc.h:2617
+ : Protocol(protocol), Loc(loc) {}
+ const ObjCProtocolDecl *getProtocol() const { return Protocol; }
+ SourceLocation getLocation() const { return Loc; }
----------------
Return non-const pointer, matching other AST nodes. (But method should be const)
================
Comment at: clang/include/clang/AST/TypeLoc.h:2620
+
+ /// Get the full source range.
+ SourceRange getSourceRange() const LLVM_READONLY {
----------------
This comment doesn't really say anything.
Maybe replace with "the source range is just the protocol name"?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2621
+ /// Evaluates true when this protocol loc is valid/non-empty.
+ explicit operator bool() const { return Protocol; }
+};
----------------
dgoldman wrote:
> sammccall wrote:
> > will we ever have invalid instances?
> From what I can tell, nope. This is explicitly used here though:
> https://github.com/llvm/llvm-project/blob/ceb5dc55c2e395c085cd9d16c4152c5a1923d7e2/clang/lib/AST/ParentMapContext.cpp#L405
> From what I can tell, nope. This is explicitly used here though:
> https://github.com/llvm/llvm-project/blob/ceb5dc55c2e395c085cd9d16c4152c5a1923d7e2/clang/lib/AST/ParentMapContext.cpp#L405
Well it's not used today, you're adding the use in this patch.
It can be solved in some other way, like pulling out an isNull template and
then overloading it for ProtocolLoc.
I don't think we should add API surface to AST nodes to satisfy brittle
templates
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
