sammccall updated this revision to Diff 252545.
sammccall marked 8 inline comments as done.
sammccall added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76741/new/
https://reviews.llvm.org/D76741
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/SemanticSelection.cpp
clang-tools-extra/clangd/SemanticSelection.h
clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
clang-tools-extra/clangd/unittests/SyncAPI.cpp
clang-tools-extra/clangd/unittests/SyncAPI.h
llvm/include/llvm/Testing/Support/Annotations.h
Index: llvm/include/llvm/Testing/Support/Annotations.h
===================================================================
--- llvm/include/llvm/Testing/Support/Annotations.h
+++ llvm/include/llvm/Testing/Support/Annotations.h
@@ -68,12 +68,14 @@
/// Crashes if there isn't exactly one.
size_t point(llvm::StringRef Name = "") const;
/// Returns the position of all points marked by ^ (or $name^) in the text.
+ /// Order matches the order within the text.
std::vector<size_t> points(llvm::StringRef Name = "") const;
/// Returns the location of the range marked by [[ ]] (or $name[[ ]]).
/// Crashes if there isn't exactly one.
Range range(llvm::StringRef Name = "") const;
/// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
+ /// They are ordered by start position within the text.
std::vector<Range> ranges(llvm::StringRef Name = "") const;
private:
Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===================================================================
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -56,8 +56,9 @@
SymbolSlab runFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req);
RefSlab getRefs(const SymbolIndex &Index, SymbolID ID);
-llvm::Expected<std::vector<Range>>
-runSemanticRanges(ClangdServer &Server, PathRef File, Position Pos);
+llvm::Expected<std::vector<SelectionRange>>
+runSemanticRanges(ClangdServer &Server, PathRef File,
+ const std::vector<Position> &Pos);
llvm::Expected<llvm::Optional<clangd::Path>>
runSwitchHeaderSource(ClangdServer &Server, PathRef File);
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -146,9 +146,10 @@
return std::move(Slab).build();
}
-llvm::Expected<std::vector<Range>>
-runSemanticRanges(ClangdServer &Server, PathRef File, Position Pos) {
- llvm::Optional<llvm::Expected<std::vector<Range>>> Result;
+llvm::Expected<std::vector<SelectionRange>>
+runSemanticRanges(ClangdServer &Server, PathRef File,
+ const std::vector<Position> &Pos) {
+ llvm::Optional<llvm::Expected<std::vector<SelectionRange>>> Result;
Server.semanticRanges(File, Pos, capture(Result));
return std::move(*Result);
}
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -24,8 +24,17 @@
namespace clang {
namespace clangd {
namespace {
+using ::testing::ElementsAre;
using ::testing::ElementsAreArray;
+// front() is SR.range, back() is outermost range.
+std::vector<Range> gatherRanges(const SelectionRange &SR) {
+ std::vector<Range> Ranges;
+ for (const SelectionRange *S = &SR; S; S = S->parent.get())
+ Ranges.push_back(S->range);
+ return Ranges;
+}
+
TEST(SemanticSelection, All) {
const char *Tests[] = {
R"cpp( // Single statement in a function body.
@@ -78,7 +87,7 @@
}]]]]
)cpp",
// Empty file.
- "^",
+ "[[^]]",
// FIXME: We should get the whole DeclStmt as a range.
R"cpp( // Single statement in TU.
[[int v = [[1^00]]]];
@@ -89,7 +98,7 @@
// FIXME: No node found associated to the position.
R"cpp( // Cursor in between spaces.
void func() {
- int v = 100 + ^ 100;
+ int v = 100 + [[^]] 100;
}
)cpp",
// Structs.
@@ -133,13 +142,13 @@
for (const char *Test : Tests) {
auto T = Annotations(Test);
auto AST = TestTU::withCode(T.code()).build();
- EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+ EXPECT_THAT(gatherRanges(llvm::cantFail(getSemanticRanges(AST, T.point()))),
ElementsAreArray(T.ranges()))
<< Test;
}
}
-TEST(SemanticSelection, RunViaClangDServer) {
+TEST(SemanticSelection, RunViaClangdServer) {
MockFSProvider FS;
MockCompilationDatabase CDB;
ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
@@ -157,15 +166,20 @@
// inp = HASH(foo(inp));
[[inp = [[HASH([[foo([[in^p]])]])]]]];
}]]]]
+ $empty[[^]]
)cpp";
Annotations SourceAnnotations(SourceContents);
FS.Files[FooCpp] = std::string(SourceAnnotations.code());
Server.addDocument(FooCpp, SourceAnnotations.code());
- auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.point());
+ auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.points());
ASSERT_TRUE(bool(Ranges))
<< "getSemanticRange returned an error: " << Ranges.takeError();
- EXPECT_THAT(*Ranges, ElementsAreArray(SourceAnnotations.ranges()));
+ ASSERT_EQ(Ranges->size(), SourceAnnotations.points().size());
+ EXPECT_THAT(gatherRanges(Ranges->front()),
+ ElementsAreArray(SourceAnnotations.ranges()));
+ EXPECT_THAT(gatherRanges(Ranges->back()),
+ ElementsAre(SourceAnnotations.range("empty")));
}
} // namespace
} // namespace clangd
Index: clang-tools-extra/clangd/SemanticSelection.h
===================================================================
--- clang-tools-extra/clangd/SemanticSelection.h
+++ clang-tools-extra/clangd/SemanticSelection.h
@@ -22,10 +22,9 @@
/// Returns the list of all interesting ranges around the Position \p Pos.
/// The interesting ranges corresponds to the AST nodes in the SelectionTree
/// containing \p Pos.
-/// Any range in the result strictly contains all the previous ranges in the
-/// result. front() is the inner most range. back() is the outermost range.
-llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
- Position Pos);
+/// If pos is not in any interesting range, return [Pos, Pos).
+llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===================================================================
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -12,6 +12,7 @@
#include "SourceCode.h"
#include "clang/AST/DeclBase.h"
#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/Error.h"
namespace clang {
@@ -26,9 +27,8 @@
}
} // namespace
-llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
- Position Pos) {
- std::vector<Range> Result;
+llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos) {
+ std::vector<Range> Ranges;
const auto &SM = AST.getSourceManager();
const auto &LangOpts = AST.getLangOpts();
@@ -56,9 +56,29 @@
Range R;
R.start = sourceLocToPosition(SM, SR->getBegin());
R.end = sourceLocToPosition(SM, SR->getEnd());
- addIfDistinct(R, Result);
+ addIfDistinct(R, Ranges);
}
- return Result;
+
+ if (Ranges.empty()) {
+ // LSP provides no way to signal "the point is not within a semantic range".
+ // Return an empty range at the point.
+ SelectionRange Empty;
+ Empty.range.start = Empty.range.end = Pos;
+ return Empty;
+ }
+
+ // Convert to the LSP linked-list representation.
+ SelectionRange Head;
+ Head.range = std::move(Ranges.front());
+ SelectionRange *Tail = &Head;
+ for (auto &Range :
+ llvm::makeMutableArrayRef(Ranges.data(), Ranges.size()).drop_front()) {
+ Tail->parent = std::make_unique<SelectionRange>();
+ Tail = Tail->parent.get();
+ Tail->range = std::move(Range);
+ }
+
+ return Head;
}
} // namespace clangd
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -292,8 +292,8 @@
Callback<std::vector<SymbolDetails>> CB);
/// Get semantic ranges around a specified position in a file.
- void semanticRanges(PathRef File, Position Pos,
- Callback<std::vector<Range>> CB);
+ void semanticRanges(PathRef File, const std::vector<Position> &Pos,
+ Callback<std::vector<SelectionRange>> CB);
/// Get all document links in a file.
void documentLinks(PathRef File, Callback<std::vector<DocumentLink>> CB);
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -654,14 +654,22 @@
WorkScheduler.runWithAST("SymbolInfo", File, std::move(Action));
}
-void ClangdServer::semanticRanges(PathRef File, Position Pos,
- Callback<std::vector<Range>> CB) {
- auto Action =
- [Pos, CB = std::move(CB)](llvm::Expected<InputsAndAST> InpAST) mutable {
- if (!InpAST)
- return CB(InpAST.takeError());
- CB(clangd::getSemanticRanges(InpAST->AST, Pos));
- };
+void ClangdServer::semanticRanges(PathRef File,
+ const std::vector<Position> &Positions,
+ Callback<std::vector<SelectionRange>> CB) {
+ auto Action = [Positions, CB = std::move(CB)](
+ llvm::Expected<InputsAndAST> InpAST) mutable {
+ if (!InpAST)
+ return CB(InpAST.takeError());
+ std::vector<SelectionRange> Result;
+ for (const auto &Pos : Positions) {
+ if (auto Range = clangd::getSemanticRanges(InpAST->AST, Pos))
+ Result.push_back(std::move(*Range));
+ else
+ return CB(Range.takeError());
+ }
+ CB(std::move(Result));
+ };
WorkScheduler.runWithAST("SemanticRanges", File, std::move(Action));
}
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -150,21 +150,6 @@
llvm::to_string(InvalidFileCount - 1) + " others)");
}
-// Converts a list of Ranges to a LinkedList of SelectionRange.
-SelectionRange render(const std::vector<Range> &Ranges) {
- if (Ranges.empty())
- return {};
- SelectionRange Result;
- Result.range = Ranges[0];
- auto *Next = &Result.parent;
- for (const auto &R : llvm::make_range(Ranges.begin() + 1, Ranges.end())) {
- *Next = std::make_unique<SelectionRange>();
- Next->get()->range = R;
- Next = &Next->get()->parent;
- }
- return Result;
-}
-
} // namespace
// MessageHandler dispatches incoming LSP messages.
@@ -1206,24 +1191,13 @@
void ClangdLSPServer::onSelectionRange(
const SelectionRangeParams &Params,
Callback<std::vector<SelectionRange>> Reply) {
- 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));
- }
Server->semanticRanges(
- Params.textDocument.uri.file(), Params.positions[0],
+ Params.textDocument.uri.file(), Params.positions,
[Reply = std::move(Reply)](
- llvm::Expected<std::vector<Range>> Ranges) mutable {
- if (!Ranges) {
+ llvm::Expected<std::vector<SelectionRange>> Ranges) mutable {
+ if (!Ranges)
return Reply(Ranges.takeError());
- }
- std::vector<SelectionRange> Result;
- Result.emplace_back(render(std::move(*Ranges)));
- return Reply(std::move(Result));
+ return Reply(std::move(*Ranges));
});
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
