[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);
     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
+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.

  rCTE Clang Tools Extra



More information about the cfe-commits mailing list