[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 4 01:38:07 PDT 2019


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





More information about the cfe-commits mailing list