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

[Sam McCall via Phabricator via cfe-commits]

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