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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 14 inline comments as done.
kadircet 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());
+    }
----------------
sammccall wrote:
> you seem to be checking this both here and in clangdlspserver. Why?
the contents in here are coming from disk, whereas the one in clangdlspserver is coming from drafts. I suppose it would make more sense to merge the two in clangdlspserver, use draft manager as content source backed by disk in case user hasn't opened that file.


================
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
----------------
sammccall wrote:
> not sure what "generates TextEdits from those" refers to.
> 
> Could this function be called "reformatEdits" or "formatAroundEdits"?
> not sure what "generates TextEdits from those" refers to.

Leftover from an old-version.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:220
+llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS,
+                                   llvm::StringMap<Edit> &ApplyEdits,
+                                   llvm::StringRef MainFilePath,
----------------
sammccall wrote:
> 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.
> 
as discussed offline for `reformatEdit` to work, made `InitialCode` in `Edit` public. As both formatting and style deduction requires access to file contents.

for `checkApplies`, it requires access to both `VFS` and `DraftMgr`and pretty special use-case for clangdlspserver, so don't think the function would carry its weight. Therefore left the check inlined.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66637