[PATCH] D119363: [clang] Add `ObjCProtocolLoc` to represent protocol references

[David Goldman via Phabricator via cfe-commits]

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