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