sammccall added a comment.
Thanks for your patience!
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:154
+getDeclAtPosition(ParsedAST &AST, SourceLocation Pos, DeclRelationSet
Relations,
+ bool *IsDependentName = nullptr) {
unsigned Offset =
AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
----------------
shovelling this IsDependentName bool around seems a bit ad-hoc. The actual node
doesn't live long enough, but can you make it an ASTNodeKind instead, and move
the isDependentName check down to locateTextually where it makes sense to be
that specific?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:355
+ bool IsDependent) {
+ // Don't use heuristics if this is a real non-dependent identifier, or not an
+ // identifier.
----------------
This sentence is hard to parse now, and also doesn't really justify the
behaviour.
Suggest leaving it alone and adding:
// Exception: dependent names, because XYZ
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:569
ASTResults = locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
- *MainFilePath, Index);
+ *MainFilePath, Index, &IsDependentName);
if (!ASTResults.empty())
----------------
the way the two (possible!) writes get combined is not obvious here.
I'd suggest using two different variables and writing `IsDependent ||
IsNearbyIdentDependent` in the usage below, if that's the intent.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:694
+
+TEST(LocateSymbol, TextualDependent) {
+ const char *Tests[] = {
----------------
this has a loop but only one test...
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:698
struct Foo {
- void uniqueMethodName();
+ void [[uniqueMethodName]]();
};
----------------
move these into the header so it can't be identifier-based heuristics? (And so
the test doesn't change behaviour when we turn that on)
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:810
struct Bar {
void $BarLoc[[uniqueMethodName]]();
};
----------------
this looks like a very close superset of TextualDependent, do we need both?
Again, these decls should move into the header I think.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76451/new/
https://reviews.llvm.org/D76451
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
