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

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.


================
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
----------------
ilya-biryukov wrote:
> 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`). 
> 
Ok, when `AllowColumnsBeyondLineLength` is false and the character value exceeds the line length, I now return an error.


================
Comment at: unittests/clangd/SourceCodeTests.cpp:37
+bool positionToOffsetCheck(StringRef Code, Position P, size_t ExpectedOffset)
+{
+  llvm::Expected<size_t> Result = positionToOffset(Code, P);
----------------
ilya-biryukov wrote:
> NIT: opening must be on the previous line
Oops, forgot to run clang-format this time.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673