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

[Sam McCall via Phabricator via cfe-commits]

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