This is an archive of the discontinued LLVM Phabricator instance.

Make positionToOffset return llvm::Expected<size_t>
ClosedPublic

Authored by simark on Mar 19 2018, 9:20 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

simark created this revision.Mar 19 2018, 9:20 PM
ilya-biryukov added inline comments.Mar 20 2018, 4:19 AM
clangd/ClangdServer.cpp
196 ↗(On Diff #139073)

NIT: maybe remove empty lines? they don't seem to add much to readability, since when prev line is indented.
up to you.

clangd/SourceCode.cpp
20 ↗(On Diff #139073)

This lambda carries its weight. Maybe inline it?

26 ↗(On Diff #139073)

NIT: don't add empty lines here too?

31 ↗(On Diff #139073)

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);
40 ↗(On Diff #139073)

NIT: remove braces around single-statement if

clangd/SourceCode.h
24 ↗(On Diff #139073)

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).

unittests/clangd/Annotations.cpp
91 ↗(On Diff #139073)

NIT: remove empty lines

unittests/clangd/SourceCodeTests.cpp
37 ↗(On Diff #139073)

NIT: opening must be on the previous line

62 ↗(On Diff #139073)

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);
simark updated this revision to Diff 139157.Mar 20 2018, 10:27 AM
simark marked 9 inline comments as done.

Address review comments

simark added inline comments.Mar 20 2018, 12:17 PM
clangd/SourceCode.h
24 ↗(On Diff #139073)

Ok, when AllowColumnsBeyondLineLength is false and the character value exceeds the line length, I now return an error.

unittests/clangd/SourceCodeTests.cpp
37 ↗(On Diff #139073)

Oops, forgot to run clang-format this time.

ilya-biryukov accepted this revision.Mar 21 2018, 4:43 AM

LGTM. See the empty line NITS, though.

clangd/ClangdServer.cpp
199 ↗(On Diff #139157)

NIT: unnecessary empty line

216 ↗(On Diff #139157)

NIT: unnecessary empty line

clangd/SourceCode.cpp
29 ↗(On Diff #139157)

NIT: unnecessary empty line

48 ↗(On Diff #139157)

NIT: unnecessary empty line

unittests/clangd/Annotations.cpp
91 ↗(On Diff #139157)

NIT: unnecessary empty line

94 ↗(On Diff #139157)

NIT: unnecessary empty line

This revision is now accepted and ready to land.Mar 21 2018, 4:43 AM
ilya-biryukov added inline comments.Mar 21 2018, 5:50 AM
clangd/SourceCode.h
41 ↗(On Diff #139157)

We should be consistent for all functions inside this fail. They may all fail for the same reasons, and it doesn't make sense for them to have a different interfaces.
Could you add a FIXME that we should update other functions to report errors?

simark updated this revision to Diff 139292.Mar 21 2018, 7:26 AM
simark marked 3 inline comments as done.

Remove spaces, add fixmes

simark marked 6 inline comments as done.Mar 21 2018, 7:27 AM
simark added inline comments.
clangd/ClangdServer.cpp
199 ↗(On Diff #139157)

In general I like spacing out the different logical blocks of code a little bit, especially after a "return", but I don't mind removing them.

ilya-biryukov added inline comments.Mar 21 2018, 7:34 AM
clangd/ClangdServer.cpp
199 ↗(On Diff #139157)

We don't really have a style rule for that, but most of clangd code does not have that many empty lines.
I personally think it's fine to have reasonable style differences in new functions, but I prefer to stick to the original author's style when refactoring code. Hence my suggestions to remove the empty lines here.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.