njames93 marked an inline comment as done. njames93 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(); ---------------- kadircet wrote: > 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. Can you elaborate on why we should disregard `InsertFromRange` if it points outside the main file. That seems like an unnecessary restriction. It's probably also safe if its a macro ID, though there's more likely to be some edge case there. As for the diagnostic. I don't think there's any value testing that. The test in `SourceCodeTests` covers inserting from range pretty well. It would be nice to test the synthetic messages, but I don't think any diagnostic in clang/clang-tidy contains just 1 fix-it that's an InsertFromRange. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97123/new/ https://reviews.llvm.org/D97123 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
