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