[PATCH] D57739: [clangd] Format tweak's replacements.

[Ilya Biryukov via Phabricator via cfe-commits]

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