sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:657
+ void VisitObjCInterfaceDecl(const ObjCInterfaceDecl *OID) {
+ if (OID->hasDefinition())
+ visitProtocolList(OID->protocols(), OID->protocol_locs());
----------------
dgoldman wrote:
> Should this be isThisDeclarationADefinition()? Is it even needed (will the
> protocol list just be empty otherwise)?
Indeed it should be isThisDeclarationADefinition().
And I think this is why we need the check - otherwise if this is a forward
decl, we'll still traverse the protocols at any visible definition.
(getReferencedProtocols() asserts, and I was calling that at some point, which
is why I originially had this check).
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:659
+ visitProtocolList(OID->protocols(), OID->protocol_locs());
+ Base::VisitObjCInterfaceDecl(OID); // Visit the interface itself.
+ }
----------------
dgoldman wrote:
> Hmm, for what? might be better to say why/what it does
Changed the comment.
(this walks back up the hierarchy, landing at VisitNamedDecl, which highlights
the primary name token)
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:668
+ /*IsDecl=*/false,
+ {OCD->getClassInterface()}});
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
dgoldman wrote:
> IIRC I think it's possible that these could be NULL if the code was invalid,
> is this NULL safe?
Good point. But checking for nulls is tedious, and guessing which cases can't
ever be null is a fool's game. Made it null-safe in
https://github.com/llvm/llvm-project/commit/7556abf82137b57be9e32475a1995f936a22cd16
instead.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:774
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ E->getSelectorStartLoc(),
+ /*IsDecl=*/false, Targets});
----------------
dgoldman wrote:
> What is this location used for? I guess it's not possible to have multiple
> Locs here like we have for a selector and you specifically handle for
> semantic highlighting below?
Yup, the abstraction only lets us report one location/token, and it's not worth
complicating for one case IMO, so we hack around it at the caller.
There are two ideas for the specific location
- it's a reasonable fallback behavior
- callers that do handle the multi-token case themselves can use this location
to match up the overlapping data. (As SemanticTokens does here and
DocumentHighlight does too albeit it uses IndexDataConsumer code rather than
findExplicitReferences)
Added a comment noting the limitation.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:927-930
+ // Possible it was something else referencing an ObjCMethodDecl.
+ // If the first token doesn't match, assume our guess was wrong.
+ if (!Locs.empty() && Locs.front() != Loc)
+ Locs.clear();
----------------
dgoldman wrote:
> Hmm, is this intentional? Not sure what else could reference it besides maybe
> a property?
I'm not sure either, but IME assuming we've covered every possible case in the
AST tends to be a bad idea :-) This is mostly a defensive check (though now you
mention it, I suppose property access might hit this.
Note that isa<ObjCMessageExpr>(OrigE) implies we're indexing a message expr,
but isa<ObjCMethodDecl>(OrigD) *doesn't* imply we're indexing a method decl!
(When indexing a message expr, OrigD is set to the target method, so the order
of if/else is significant here).
This makes me feel like it would be quite plausible to go off the rails without
this check.
I've tweaked the comments a bit here to reinforce this is a sanity check.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97617/new/
https://reviews.llvm.org/D97617
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
