simark updated this revision to Diff 139292.
simark marked 3 inline comments as done.
simark added a comment.
Remove spaces, add fixmes
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/CMakeLists.txt
unittests/clangd/SourceCodeTests.cpp
Index: unittests/clangd/SourceCodeTests.cpp
===================================================================
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -7,14 +7,19 @@
//
//===----------------------------------------------------------------------===//
#include "SourceCode.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
namespace clang{
namespace clangd {
namespace {
+using llvm::Failed;
+using llvm::HasValue;
+
MATCHER_P2(Pos, Line, Col, "") {
return arg.line == Line && arg.character == Col;
}
@@ -33,30 +38,57 @@
TEST(SourceCodeTests, PositionToOffset) {
// line out of bounds
- EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), Failed());
// 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_THAT_EXPECTED(positionToOffset(File, position(0, -1)),
+ Failed()); // out of range
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 0)),
+ HasValue(0)); // first character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 3)),
+ HasValue(3)); // middle character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 6)),
+ HasValue(6)); // last character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7)),
+ HasValue(7)); // the newline itself
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7), false),
+ HasValue(7));
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8)),
+ HasValue(7)); // out of range
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8), false),
+ Failed()); // 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_THAT_EXPECTED(positionToOffset(File, position(1, -1)),
+ Failed()); // out of range
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 0)),
+ HasValue(8)); // first character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3)),
+ HasValue(11)); // middle character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3), false),
+ HasValue(11));
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 6)),
+ HasValue(14)); // last character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 7)),
+ HasValue(15)); // the newline itself
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8)),
+ HasValue(15)); // out of range
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8), false),
+ Failed()); // 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_THAT_EXPECTED(positionToOffset(File, position(2, -1)),
+ Failed()); // out of range
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 0)),
+ HasValue(16)); // first character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 3)),
+ HasValue(19)); // middle character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 7)),
+ HasValue(23)); // last character
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 8)),
+ HasValue(24)); // EOF
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 9), false),
+ Failed()); // out of range
// line out of bounds
- EXPECT_EQ(24u, positionToOffset(File, position(3, 1)));
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(3, 0)), Failed());
+ EXPECT_THAT_EXPECTED(positionToOffset(File, position(3, 1)), Failed());
}
TEST(SourceCodeTests, OffsetToPosition) {
Index: unittests/clangd/CMakeLists.txt
===================================================================
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -43,4 +43,5 @@
clangTooling
clangToolingCore
LLVMSupport
+ LLVMTestingSupport
)
Index: unittests/clangd/Annotations.cpp
===================================================================
--- unittests/clangd/Annotations.cpp
+++ unittests/clangd/Annotations.cpp
@@ -86,7 +86,11 @@
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
@@ -22,12 +22,27 @@
namespace clangd {
/// Turn a [line, column] pair into an offset in Code.
-size_t positionToOffset(llvm::StringRef Code, Position P);
+///
+/// If the character value is greater than the line length, the behavior depends
+/// on AllowColumnsBeyondLineLength:
+///
+/// - if true: default back to the end of the line
+/// - if false: return an error
+///
+/// If the line number is greater than the number of lines in the document,
+/// always return an error.
+///
+/// The returned value is in the range [0, Code.size()].
+llvm::Expected<size_t>
+positionToOffset(llvm::StringRef Code, Position P,
+ bool AllowColumnsBeyondLineLength = true);
/// Turn an offset in Code into a [line, column] pair.
+/// FIXME: This should return an error if the offset is invalid.
Position offsetToPosition(llvm::StringRef Code, size_t Offset);
/// Turn a SourceLocation into a [line, column] pair.
+/// FIXME: This should return an error if the location is invalid.
Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
// Converts a half-open clang source range to an LSP range.
Index: clangd/SourceCode.cpp
===================================================================
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -9,23 +9,43 @@
#include "SourceCode.h"
#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
namespace clang {
namespace clangd {
using namespace llvm;
-size_t positionToOffset(StringRef Code, Position P) {
+llvm::Expected<size_t> positionToOffset(StringRef Code, Position P,
+ bool AllowColumnsBeyondLineLength) {
if (P.line < 0)
- return 0;
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Line value can't be negative ({0})", P.line),
+ llvm::errc::invalid_argument);
+ if (P.character < 0)
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Character value can't be negative ({0})", P.character),
+ llvm::errc::invalid_argument);
size_t StartOfLine = 0;
for (int I = 0; I != P.line; ++I) {
size_t NextNL = Code.find('\n', StartOfLine);
if (NextNL == StringRef::npos)
- return Code.size();
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Line value is out of range ({0})", P.line),
+ llvm::errc::invalid_argument);
StartOfLine = NextNL + 1;
}
+
+ size_t NextNL = Code.find('\n', StartOfLine);
+ if (NextNL == StringRef::npos)
+ NextNL = Code.size();
+
+ if (StartOfLine + P.character > NextNL && !AllowColumnsBeyondLineLength)
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Character value is out of range ({0})", P.character),
+ llvm::errc::invalid_argument);
// 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,13 @@
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();
+ return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});
}
llvm::Expected<tooling::Replacements> ClangdServer::formatFile(StringRef Code,
@@ -205,11 +209,13 @@
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
