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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
