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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D57739#1384844 <https://reviews.llvm.org/D57739#1384844>, @ilya-biryukov wrote:

> 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.


This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the `FormatStyle` somehow), I think this might be done in another patch.



================
Comment at: clangd/ClangdServer.cpp:366
+  auto Style = getFormatStyle(Code, File);
+  if (!Style)
+    return;
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > hokein wrote:
> > > not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.
> > You should use `getFormatStyleForFile` from SourceCode.h
> > not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.
> Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.
> You should use getFormatStyleForFile from SourceCode.h

Thanks for pointing it out! Didn't notice this method. I think we need to cache it somehow rather than calling this method every time.

> Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.

I switched to use getFormatStyleForFile, it seems a reasonable API. 


================
Comment at: clangd/ClangdServer.h:267
+  /// slow. Think of a way to cache this.
+  llvm::Expected<format::FormatStyle> getFormatStyle(llvm::StringRef Code,
+                                                     PathRef File);
----------------
ilya-biryukov wrote:
> NIT: could we swap the parameters for consistency with the other functions in this class (i.e. `File` goes first)
removed, not needed any more.


================
Comment at: unittests/clangd/TweakTests.cpp:82
+llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input,
+                                  bool Format) {
   Annotations Code(Input);
----------------
ilya-biryukov wrote:
> NIT: remove the parameter, this should be the default.
> At least until we run into the checks that deliberately don't want the formatting
I'm ok to make this change. I tempt to keep it because it would make writing test code easier -- if we always format the code, we might spend time on figuring out the format diff between `Before` and `After`, which is not worth.


================
Comment at: unittests/clangd/TweakTests.cpp:110
+        Code.code(), *Replacements,
+        clang::format::getGoogleStyle(::clang::format::FormatStyle::LK_Cpp));
+    if (!Replacements)
----------------
ilya-biryukov wrote:
> NIT: maybe prefer LLVM style? To avoid context-switching.
good catch.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57739/new/

https://reviews.llvm.org/D57739