sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:647
+ WE.changes.emplace();
+ auto FS = FSProvider.getFileSystem();
+ for (const auto &It : R->ApplyEdits) {
----------------
please pull out a method for this part
e.g. `Error validateEdits(...)`
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:654
+ if (!It.second.canApplyTo(*Draft))
+ return Reply((It.first() + " needs to be saved").str());
+ }
----------------
As these are absolute paths, we probably want to invert the error message for
readability, and at least count the files.
e.g.
"Files must be saved first: path/to/Foo.cpp (and 3 others)"
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+ if (llvm::Error Err = reformatEdit(E, Style)) {
+ llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase &EIB) {
+ elog("Failed to format {0}: {1}", It.first(), EIB.message());
----------------
isn't this just
`elog("Failed to format {0}: {1}", std::move(Err))`
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:856
+
+bool Edit::canApplyTo(llvm::StringRef Code) const {
+ auto LHS = llvm::MemoryBuffer::getMemBuffer(Code);
----------------
documentation
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:870
+
+ // We only allow trailing empty lines.
+ while (!LHSIt.is_at_eof()) {
----------------
why?
================
Comment at: clang-tools-extra/clangd/SourceCode.h:42
+/// A set of edits generated for a single file. Can verify whether it is safe
to
+/// apply these edits to a code block.
----------------
move this class near the related code? (`replacementToEdit` etc)
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:90
+ assert(FE && "Generating edits for unknown file!");
+ llvm::StringRef FilePath = FE->getName();
+ Edit Ed(SM.getBufferData(FID), std::move(Replacements));
----------------
how do you know this is the correct/absolute path for the file? Can we add
tests?
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:73
llvm::Optional<std::string> ShowMessage;
- /// An edit to apply to the input file.
- llvm::Optional<tooling::Replacements> ApplyEdit;
+ /// A mapping from absolute file path to edits.
+ llvm::StringMap<Edit> ApplyEdits;
----------------
is it possible to be more specific than "absolute"?
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager &SM, FileID FID,
+ tooling::Replacements Replacements);
----------------
what will this be used for?
You can use at most one of these Effect factories (unless we're going to do
some convoluted merge thing), so this seems useful if you've got exactly one
edit to a non-main file.
It seems more useful to return a pair<string, Edit> which could be inserted
into a map
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager &SM, FileID FID,
+ tooling::Replacements Replacements);
----------------
sammccall wrote:
> what will this be used for?
>
> You can use at most one of these Effect factories (unless we're going to do
> some convoluted merge thing), so this seems useful if you've got exactly one
> edit to a non-main file.
>
> It seems more useful to return a pair<string, Edit> which could be inserted
> into a map
why are these moved out of the Effect class?
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:133
+
+/// Convinience wrapper around above.
+Tweak::Effect mainFileEdit(const SourceManager &SM,
----------------
nit: convenience
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66637/new/
https://reviews.llvm.org/D66637
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
