[PATCH] D50958: [clangd] Implement findReferences function
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 3 08:35:43 PDT 2018
sammccall added a comment.
Looking forward to using this!
Unless you object, I'd like to address the remaining comments (and get a review), so you can make the most of your leave!
================
Comment at: clangd/XRefs.cpp:333
+
+ DeclOccurrences[D].emplace_back(FileLoc, Roles);
+ return true;
----------------
why is this a map keyed by D? the keys are never used, the result is always flattened into a vector.
================
Comment at: clangd/XRefs.cpp:361
+/// Find symbol occurrences that a given whilelist of declarations refers to.
+class OccurrencesFinder : public OccurrenceCollector {
+public:
----------------
why is this a subclass rather than just using OccurrenceCollector and putting the finish() code in findReferences()?
================
Comment at: clangd/XRefs.cpp:378
+ }
+ // Deduplicate results.
+ std::sort(Occurrences.begin(), Occurrences.end());
----------------
out of curiosity: how do we actually get duplicates?
================
Comment at: clangd/XRefs.cpp:388
+
+/// Finds document highlights that a given whitelist of declarations refers to.
+class DocumentHighlightsFinder : public OccurrenceCollector {
----------------
as above, why does this need to be a subclass
================
Comment at: clangd/XRefs.cpp:718
+ getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+ // Identified symbols at a specific position.
+ auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
----------------
(comment just echoes code)
================
Comment at: clangd/XRefs.cpp:721
+
+ // For local symbols (e.g. symbols that are only visiable in main file,
+ // symbols defined in function body), we can get complete references by
----------------
visiable -> visible
================
Comment at: clangd/XRefs.cpp:724
+ // traversing the AST, and we don't want to make unnecessary queries to the
+ // index. Howerver, we don't have a reliable way to distinguish file-local
+ // symbols. We conservatively consider function-local symbols.
----------------
we can check isExternallyVisible, I think?
maybe it's not worth it, but the comment as it stands doesn't seem accurate
================
Comment at: unittests/clangd/XRefsTests.cpp:1193
+ )";
+ MockIndex Index;
+ for (const char *Test : Tests) {
----------------
can we use a simple real implementation `TU.index()` instead of mocking here?
I think this is an artifact of the fact that TestTU doesn't actually expose occurrences in the index, can we fix that?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50958
More information about the cfe-commits
mailing list