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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:601
+          llvm::inconvertibleErrorCode(),
+          "File contents differ on disk for %s, please save", FilePath.data());
+    }
----------------
you seem to be checking this both here and in clangdlspserver. Why?


================
Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {
----------------
nit: drop "represents"


================
Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {
----------------
sammccall wrote:
> nit: drop "represents"
nit: this could also describe Replacements, vector<TextEdit>, etc. Motivate this class a little more?


================
Comment at: clang-tools-extra/clangd/SourceCode.h:215
 
+/// Formats the edits in \p ApplyEdits and generates TextEdits from those.
+/// Ensures the files in FS are consistent with the files used while generating
----------------
not sure what "generates TextEdits from those" refers to.

Could this function be called "reformatEdits" or "formatAroundEdits"?


================
Comment at: clang-tools-extra/clangd/SourceCode.h:220
+llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS,
+                                   llvm::StringMap<Edit> &ApplyEdits,
+                                   llvm::StringRef MainFilePath,
----------------
This signature is confusing: we pass code in *three* different ways (FS, Edits, and MainFileCode). Much of this is because we put the loop (and therefore all special cases) inside this function.
The logic around the way the VFS is mapped for the main file vs others doesn't really belong in this layer. Neither does "please save"...

It seems this wants to be something simpler/smaller like `reformatEdit(const Edit&, const Style&)` that could be called from ClangdServer. There's probably another helper like `checkApplies(const Edit&, VFS*)` that would go in ClangdLSPServer.



================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74
+    /// A mapping from absolute file path to edits.
+    llvm::Optional<llvm::StringMap<Edit>> ApplyEdits;
 
----------------
what's the difference between `None` and an empty map?


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:76
 
-    static Effect applyEdit(tooling::Replacements R) {
+    void addEdit(StringRef FilePath, Edit Ed) {
+      assert(ApplyEdits);
----------------
(if the null map case goes away, this can too)


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:81
+
+    static Effect applyEdit(StringRef FilePath, Edit Ed) {
       Effect E;
----------------
This greatly increases the burden of callers of this function, who mostly want to edit the current file.
 - need to provide a fully-qualified name (I don't think the implementations in this patch actually do that)
 - need to construct an Edit

can we provide an `Effect mainFileEdit(const SourceManager&, Replacements)`? and maybe `Effect fileEdit(const SourceManager&, FileID, Replacements)` though maybe the latter doesn't cover enough cases to pull its weight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66637