dgoldman added a comment.
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?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2611
+class ObjCProtocolLoc {
+ const ObjCProtocolDecl *Protocol = nullptr;
+ SourceLocation Loc = SourceLocation();
----------------
sammccall wrote:
> Also add a public `getLocation()` accessor for ProtocolLoc? (That returns
> SourceLocation)
this was already there? did you want something else?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2617
+ : Protocol(protocol), Loc(loc) {}
+ const ObjCProtocolDecl *getProtocol() const { return Protocol; }
+ SourceLocation getLocation() const { return Loc; }
----------------
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).
================
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; }
+};
----------------
sammccall wrote:
> 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
Gotcha, I'm relatively new to templates but took a stab at isNull - LMK what
you think/if there's a better way.
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
