[PATCH] D57739: [clangd] Format tweak's replacements.
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 5 03:00:58 PST 2019
ilya-biryukov added a comment.
Could we move the code to the `Tweak` itself, so that all the callers would get the formatting? I.e.
class Tweak {
Replacements apply() { // This is now non-virtual.
auto Replacements = doApply();
return cleanupAndFormat(Replacements);
}
protected:
virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};
This would ensure the callers don't need to deal with the formatting on their own.
================
Comment at: clangd/ClangdServer.cpp:362
-void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
+void ClangdServer::applyTweak(StringRef Code, PathRef File, Range Sel,
+ StringRef TweakID,
----------------
We can get the contents from `InpAST.Inputs.Contents`, no need to add an extra parameter.
Moving `getFormatStyle()` into the callback body should be ok.
================
Comment at: clangd/ClangdServer.cpp:500
+llvm::Expected<format::FormatStyle>
+ClangdServer::getFormatStyle(llvm::StringRef Code, PathRef File) {
+ return format::getStyle(format::DefaultFormatStyle, File,
----------------
Could this function get a `vfs::FileSystem` as a parameter?
Would server as a hint to the readers that it accesses the filesystem and allow to reuse vfs instances when they're already available (the apply tweaks case)
================
Comment at: clangd/ClangdServer.h:267
+ /// slow. Think of a way to cache this.
+ llvm::Expected<format::FormatStyle> getFormatStyle(llvm::StringRef Code,
+ PathRef File);
----------------
NIT: could we swap the parameters for consistency with the other functions in this class (i.e. `File` goes first)
================
Comment at: clangd/Format.h:29
+} // namespace clang
\ No newline at end of file
----------------
NIT: add newline at the end of file.
================
Comment at: unittests/clangd/TweakTests.cpp:82
+llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input,
+ bool Format) {
Annotations Code(Input);
----------------
NIT: remove the parameter, this should be the default.
At least until we run into the checks that deliberately don't want the formatting
================
Comment at: unittests/clangd/TweakTests.cpp:110
+ Code.code(), *Replacements,
+ clang::format::getGoogleStyle(::clang::format::FormatStyle::LK_Cpp));
+ if (!Replacements)
----------------
NIT: maybe prefer LLVM style? To avoid context-switching.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57739/new/
https://reviews.llvm.org/D57739
More information about the cfe-commits
mailing list