[PATCH] D97123: [clangd] Support FixIts that use InsertFromRange instead of inserting raw text

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:555
+    auto Insert = Lexer::getSourceText(FixIt.InsertFromRange, M, L, &Invalid);
+    if (!Invalid)
+      Result.newText = Insert.str();
----------------
njames93 wrote:
> kadircet wrote:
> > it feels like correct semantics would be to make this function fail now. as the resulting value will likely be used for editing the source code and we don't want to propagate some garbage.
> While I agree with this in principal. We currently don't handle the case where RemoveRange doesn't correspond to a FileCharRange which would likely need propagating. Though likely in a separate patch. 
i discussed the situation with sam offline (as you've also noticed most of the sourcelocation -> lsp conversations within this file doesn't really check for errors), and it looks like this was intentional. we are trying to cover as much ground as possible in diagnostics.cpp, especially in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Diagnostics.cpp#L677. we should do the same for `InsertFromRange`. all the macroarg logic isn't necessary (until we notice it is), so just bailing out when the `InsertFromRange` is a macroid or outside mainfile in `AddFix` should be enough.

as for testing it, looks like you can invoke a fixit with `InsertFromRange` via:
```
struct Foo {};
struct Bar : public [[noreturn]] Foo {};
```

it would be nice to also check with an attribute coming from a macro expansions.


================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:826
+  Expected<SourceRange> V2Char = sourceRangeInMainFile(SM, Code.range("V2"));
+  auto V1Tok = sourceLocationInMainFile(SM, Code.point("V1"));
+  auto V2Tok = sourceLocationInMainFile(SM, Code.point("V2"));
----------------
nit: I would just wrap these in `llvm::cantFail`, similarly for `sourceRangeInMainFile`. these are just tests, and we don't really expect them to be outside mainfile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97123/new/

https://reviews.llvm.org/D97123