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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 8 02:22:41 PST 2019


sammccall added a comment.

In D57739#1385144 <https://reviews.llvm.org/D57739#1385144>, @hokein wrote:

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


I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739





More information about the cfe-commits mailing list