[PATCH] D97123: [clangd] Support FixIts that use InsertFromRange instead of inserting raw text
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 23 00:56:50 PST 2021
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
More information about the cfe-commits
mailing list