simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov,
klimek.
simark added a reviewer: ilya-biryukov.
To implement incremental document syncing, we want to verify that the
ranges provided by the front-end are valid. Currently, positionToOffset
deals with invalid Positions by returning 0 or Code.size(), which are
two valid offsets.
Instead, return an llvm:Expected<size_t> with an error if the position
is invalid. According to the LSP, if the character value exceeds the
number of characters of the given line, it should default back to the
end of the line, so this is what I did. However, I considered a line
number larger than the number of lines in the file to be an error.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44673
Files:
clangd/ClangdServer.cpp
clangd/SourceCode.cpp
clangd/SourceCode.h
unittests/clangd/Annotations.cpp
unittests/clangd/SourceCodeTests.cpp
Index: unittests/clangd/SourceCodeTests.cpp
===================================================================
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//
#include "SourceCode.h"
#include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -31,32 +32,58 @@
return Pos;
}
+/// Return true if the return value of positionToOffset is equal to ExpectedOffset.
+bool positionToOffsetCheck(StringRef Code, Position P, size_t ExpectedOffset)
+{
+ llvm::Expected<size_t> Result = positionToOffset(Code, P);
+ assert(Result);
+ return *Result == ExpectedOffset;
+}
+
+void ignoreError(llvm::Error Err) {
+ handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
+}
+
+/// Return true if the call to positionToOffset generates an error.
+bool positionToOffsetFails(StringRef Code, Position P)
+{
+ llvm::Expected<size_t> Result = positionToOffset(Code, P);
+
+ bool HasError = !Result;
+
+ if (HasError)
+ ignoreError(Result.takeError());
+
+ return HasError;
+}
+
TEST(SourceCodeTests, PositionToOffset) {
// line out of bounds
- EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+ EXPECT_TRUE(positionToOffsetFails(File, position(-1, 2)));
// first line
- EXPECT_EQ(0u, positionToOffset(File, position(0, -1))); // out of range
- EXPECT_EQ(0u, positionToOffset(File, position(0, 0))); // first character
- EXPECT_EQ(3u, positionToOffset(File, position(0, 3))); // middle character
- EXPECT_EQ(6u, positionToOffset(File, position(0, 6))); // last character
- EXPECT_EQ(7u, positionToOffset(File, position(0, 7))); // the newline itself
- EXPECT_EQ(8u, positionToOffset(File, position(0, 8))); // out of range
+ EXPECT_TRUE(positionToOffsetFails(File, position(0, -1))); // out of range
+ EXPECT_TRUE(positionToOffsetCheck(File, position(0, 0), 0)); // first character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(0, 3), 3)); // middle character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(0, 6), 6)); // last character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(0, 7), 7)); // the newline itself
+ EXPECT_TRUE(positionToOffsetCheck(File, position(0, 8), 7)); // out of range
// middle line
- EXPECT_EQ(8u, positionToOffset(File, position(1, -1))); // out of range
- EXPECT_EQ(8u, positionToOffset(File, position(1, 0))); // first character
- EXPECT_EQ(11u, positionToOffset(File, position(1, 3))); // middle character
- EXPECT_EQ(14u, positionToOffset(File, position(1, 6))); // last character
- EXPECT_EQ(15u, positionToOffset(File, position(1, 7))); // the newline itself
- EXPECT_EQ(16u, positionToOffset(File, position(1, 8))); // out of range
+ EXPECT_TRUE(positionToOffsetFails(File, position(1, -1))); // out of range
+ EXPECT_TRUE(positionToOffsetCheck(File, position(1, 0), 8)); // first character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(1, 3), 11)); // middle character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(1, 6), 14)); // last character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(1, 7), 15)); // the newline itself
+ EXPECT_TRUE(positionToOffsetCheck(File, position(1, 8), 15)); // out of range
// last line
- EXPECT_EQ(16u, positionToOffset(File, position(2, -1))); // out of range
- EXPECT_EQ(16u, positionToOffset(File, position(2, 0))); // first character
- EXPECT_EQ(19u, positionToOffset(File, position(2, 3))); // middle character
- EXPECT_EQ(23u, positionToOffset(File, position(2, 7))); // last character
- EXPECT_EQ(24u, positionToOffset(File, position(2, 8))); // EOF
- EXPECT_EQ(24u, positionToOffset(File, position(2, 9))); // out of range
+ EXPECT_TRUE(positionToOffsetFails(File, position(2, -1))); // out of range
+ EXPECT_TRUE(positionToOffsetCheck(File, position(2, 0), 16)); // first character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(2, 3), 19)); // middle character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(2, 7), 23)); // last character
+ EXPECT_TRUE(positionToOffsetCheck(File, position(2, 8), 24)); // EOF
+ EXPECT_TRUE(positionToOffsetCheck(File, position(2, 8), 24)); // out of range
// line out of bounds
- EXPECT_EQ(24u, positionToOffset(File, position(3, 1)));
+ EXPECT_TRUE(positionToOffsetFails(File, position(3, 0)));
+ EXPECT_TRUE(positionToOffsetFails(File, position(3, 1)));
}
TEST(SourceCodeTests, OffsetToPosition) {
Index: unittests/clangd/Annotations.cpp
===================================================================
--- unittests/clangd/Annotations.cpp
+++ unittests/clangd/Annotations.cpp
@@ -86,7 +86,13 @@
std::pair<std::size_t, std::size_t>
Annotations::offsetRange(llvm::StringRef Name) const {
auto R = range(Name);
- return {positionToOffset(Code, R.start), positionToOffset(Code, R.end)};
+ llvm::Expected<size_t> Start = positionToOffset(Code, R.start);
+ llvm::Expected<size_t> End = positionToOffset(Code, R.end);
+
+ assert(Start);
+ assert(End);
+
+ return {*Start, *End};
}
} // namespace clangd
Index: clangd/SourceCode.h
===================================================================
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -21,8 +21,13 @@
namespace clangd {
-/// Turn a [line, column] pair into an offset in Code.
-size_t positionToOffset(llvm::StringRef Code, Position P);
+/// Turn a [line, column] pair into an offset in Code according to the LSP
+/// definition of a Position. If the character value is greater than the line
+/// length, it defaults back to the line length. If the line number is greater
+/// than the number of lines in the document, return an error.
+///
+/// The returned value is in the range [0, Code.size()].
+llvm::Expected<size_t> positionToOffset(llvm::StringRef Code, Position P);
/// Turn an offset in Code into a [line, column] pair.
Position offsetToPosition(llvm::StringRef Code, size_t Offset);
Index: clangd/SourceCode.cpp
===================================================================
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -9,23 +9,39 @@
#include "SourceCode.h"
#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/Errc.h"
namespace clang {
namespace clangd {
using namespace llvm;
-size_t positionToOffset(StringRef Code, Position P) {
+llvm::Expected<size_t> positionToOffset(StringRef Code, Position P) {
+ auto Err = [] (const Twine &S) {
+ return llvm::make_error<llvm::StringError>(S, llvm::errc::invalid_argument);
+ };
+
if (P.line < 0)
- return 0;
+ return Err(llvm::formatv("Line value can't be negative ({0})", P.line));
+
+ if (P.character < 0)
+ return Err(llvm::formatv("Character value can't be negative ({0})", P.character));
+
size_t StartOfLine = 0;
+ size_t NextNL = Code.find('\n', StartOfLine);
for (int I = 0; I != P.line; ++I) {
- size_t NextNL = Code.find('\n', StartOfLine);
if (NextNL == StringRef::npos)
- return Code.size();
+ return Err(llvm::formatv("Line value is out of range ({0})", P.line));
StartOfLine = NextNL + 1;
+ NextNL = Code.find('\n', StartOfLine);
}
+
+ if (NextNL == StringRef::npos) {
+ NextNL = Code.size();
+ }
+
// FIXME: officially P.character counts UTF-16 code units, not UTF-8 bytes!
- return std::min(Code.size(), StartOfLine + std::max(0, P.character));
+ return std::min(NextNL, StartOfLine + P.character);
}
Position offsetToPosition(StringRef Code, size_t Offset) {
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -190,9 +190,16 @@
llvm::Expected<tooling::Replacements>
ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {
- size_t Begin = positionToOffset(Code, Rng.start);
- size_t Len = positionToOffset(Code, Rng.end) - Begin;
- return formatCode(Code, File, {tooling::Range(Begin, Len)});
+ llvm::Expected<size_t> Begin = positionToOffset(Code, Rng.start);
+ if (!Begin)
+ return Begin.takeError();
+
+ llvm::Expected<size_t> End = positionToOffset(Code, Rng.end);
+ if (!End)
+ return End.takeError();
+
+ size_t Len = *End - *Begin;
+ return formatCode(Code, File, {tooling::Range(*Begin, Len)});
}
llvm::Expected<tooling::Replacements> ClangdServer::formatFile(StringRef Code,
@@ -205,11 +212,14 @@
ClangdServer::formatOnType(StringRef Code, PathRef File, Position Pos) {
// Look for the previous opening brace from the character position and
// format starting from there.
- size_t CursorPos = positionToOffset(Code, Pos);
- size_t PreviousLBracePos = StringRef(Code).find_last_of('{', CursorPos);
+ llvm::Expected<size_t> CursorPos = positionToOffset(Code, Pos);
+ if (!CursorPos)
+ return CursorPos.takeError();
+
+ size_t PreviousLBracePos = StringRef(Code).find_last_of('{', *CursorPos);
if (PreviousLBracePos == StringRef::npos)
- PreviousLBracePos = CursorPos;
- size_t Len = CursorPos - PreviousLBracePos;
+ PreviousLBracePos = *CursorPos;
+ size_t Len = *CursorPos - PreviousLBracePos;
return formatCode(Code, File, {tooling::Range(PreviousLBracePos, Len)});
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
