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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits