usaxena95 added a comment. Mostly looks good. Few nits. Thanks.
================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1194-1201 if (Params.positions.size() != 1) { elog("{0} positions provided to SelectionRange. Supports exactly one " "position.", Params.positions.size()); return Reply(llvm::make_error<LSPError>( "SelectionRange supports exactly one position", ErrorCode::InvalidRequest)); ---------------- You can remove this check now. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:671 + } + return CB(std::move(Result)); + }; ---------------- nit: return statement can be omitted. ================ Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:65 + // Return an empty range at the point. + SelectionRange Dummy; + Dummy.range.start = Dummy.range.end = Pos; ---------------- nit: s/Dummy/Empty ? ================ Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:74 + SelectionRange *Tail = &Head; + for (auto &Range : llvm::makeArrayRef(Ranges).drop_front()) { + Tail->parent = std::make_unique<SelectionRange>(); ---------------- nit:`Range` can be made a `const` ref. ================ Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:175 - auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.point()); + auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.points()); ASSERT_TRUE(bool(Ranges)) ---------------- I think it would be better to name the two points in the test and explicitly specify their order in the request (instead of relying on `SourceAnnotations.points()`). Annotations::points doesn't guarantee order in its contract. Even if the current implementation does, it would be better to be explicit and not rely on it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76741/new/ https://reviews.llvm.org/D76741 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits