[PATCH] D97617: [clangd] ObjC fixes for semantic highlighting and xref highlights
David Goldman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 1 10:31:07 PST 2021
dgoldman added a comment.
In D97617#2592424 <https://reviews.llvm.org/D97617#2592424>, @sammccall wrote:
> (not really sure why this suddenly seemed important to me, but if you don't have semantic highlighting on, you're missing out!)
Thanks!
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:657
+ void VisitObjCInterfaceDecl(const ObjCInterfaceDecl *OID) {
+ if (OID->hasDefinition())
+ visitProtocolList(OID->protocols(), OID->protocol_locs());
----------------
Should this be isThisDeclarationADefinition()? Is it even needed (will the protocol list just be empty otherwise)?
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:659
+ visitProtocolList(OID->protocols(), OID->protocol_locs());
+ Base::VisitObjCInterfaceDecl(OID); // Visit the interface itself.
+ }
----------------
Hmm, for what? might be better to say why/what it does
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:668
+ /*IsDecl=*/false,
+ {OCD->getClassInterface()}});
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
IIRC I think it's possible that these could be NULL if the code was invalid, is this NULL safe?
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:687-689
+ if (OPD->hasDefinition())
+ visitProtocolList(OPD->protocols(), OPD->protocol_locs());
+ Base::VisitObjCProtocolDecl(OPD); // Visit the protocol itself.
----------------
Same comments here as the interface code above
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:774
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ E->getSelectorStartLoc(),
+ /*IsDecl=*/false, Targets});
----------------
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?
================
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();
----------------
Hmm, is this intentional? Not sure what else could reference it besides maybe a property?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97617/new/
https://reviews.llvm.org/D97617
More information about the cfe-commits
mailing list