[PATCH] D44673: Make positionToOffset return llvm::Expected<size_t>

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:196
+    return Begin.takeError();
+
+  llvm::Expected<size_t> End = positionToOffset(Code, Rng.end);
----------------
NIT: maybe remove empty lines? they don't seem to add much to readability, since when prev line is indented.
up to you.


================
Comment at: clangd/SourceCode.cpp:20
+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);
----------------
This lambda carries its weight. Maybe inline it?


================
Comment at: clangd/SourceCode.cpp:26
+    return Err(llvm::formatv("Line value can't be negative ({0})", P.line));
+
+  if (P.character < 0)
----------------
NIT: don't add empty lines here too?


================
Comment at: clangd/SourceCode.cpp:31
   size_t StartOfLine = 0;
+  size_t NextNL = Code.find('\n', StartOfLine);
   for (int I = 0; I != P.line; ++I) {
----------------
I find the previous code more readable, the more values are "outputs" of the loop, the trickier it gets to read.
I suggest keeping `NextNL` inside a loop:
```
size_t StartOfLine = 0;
for (int I = 0; I != P.line; ++I) {
  size_t NextNL = Code.find('\n', StartOfLine);
  if (NextNL == StringRef::npos)
     return Err(...);
  StartOfLine = NextNL + 1;
}
size_t LastColumn = Code.fine('\n', StartOfLine);
if (LastColumn == StringRef::npos)
  LastColumn = Code.size();
return std::min(LastColumn, StartOfLine = P.character);
```


================
Comment at: clangd/SourceCode.cpp:40
+  if (NextNL == StringRef::npos) {
+    NextNL = Code.size();
+  }
----------------
NIT: remove braces around single-statement if


================
Comment at: clangd/SourceCode.h:24
 
-/// 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
----------------
Reverting to line length for larger column numbers seems to be the wrong thing to do for content changes, even though LSP does not distinguish this case.
The fundamental difference is that position provided as inputs to features are caret or selection positions and it makes sense for them to span non-existing columns if the editors allows to put the caret there. Text changes, on the other hand, should be generated from proper offsets inside a string and it does not make sense for their columns to be out of range.
I suggest to add a param with default argument to the function to control this behavior (`bool AllowColumnsBeyondLineLength = true`). 



================
Comment at: unittests/clangd/Annotations.cpp:91
+  llvm::Expected<size_t> End = positionToOffset(Code, R.end);
+
+  assert(Start);
----------------
NIT: remove empty lines


================
Comment at: unittests/clangd/SourceCodeTests.cpp:37
+bool positionToOffsetCheck(StringRef Code, Position P, size_t ExpectedOffset)
+{
+  llvm::Expected<size_t> Result = positionToOffset(Code, P);
----------------
NIT: opening must be on the previous line


================
Comment at: unittests/clangd/SourceCodeTests.cpp:62
   // line out of bounds
-  EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+  EXPECT_TRUE(positionToOffsetFails(File, position(-1, 2)));
   // first line
----------------
Use matchers from `include/llvm/Testing/Support/Error.h` instead of the helper functions:

```
using llvm::Failed;
using llvm::HasValue;
using testing::Eq;

// ....

EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), HasValue(Eq(2)));
EXPECT_THAT_EXPECTED(positionToOffset(File, position(100, 200)), Failed);


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673