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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D57739#1392453 <https://reviews.llvm.org/D57739#1392453>, @sammccall wrote:

> In D57739#1390374 <https://reviews.llvm.org/D57739#1390374>, @ilya-biryukov wrote:
>
> > In D57739#1390321 <https://reviews.llvm.org/D57739#1390321>, @sammccall wrote:
> >
> > > It's not about stability or whether the functionality is desired, but layering.
> > >  Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
> > >  Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.
> >
> >
> > My argument is that it's better to provide formatting by default in the public interface for **applying a single tweak**.
> >  I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.
>
>
> If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.
>
> For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.
>
> > My concerns are code duplication and ease of use for the clients. Having both `apply` and `applyAndFormat` (the latter being a non-virtual or a free-standing utility function) in the public interface of the `Tweak` would totally do it for me.
>
> I'd be happy with `applyAndFormat` as a free function - my main concern is that formatting not be part of the scope of the class.


Given that clangdServer is the only client of `Tweaks` (if we don't do format in tests), I think it is fine to move out `applyAndFormat` functionality from `Tweaks`. I somehow agree that `applyAndFormat` is a standalone function.

>> However, I also think tests should format by default, not sure we agree on this.
>>  WDYT?
> 
> I'd rather they didn't format, but I don't think it will matter much and I'm happy either way.
> 
> (Where it does matter, I'd rather have the differences between input/output be a result of the tweak replacements, not of different formatting triggered by a different token sequence.
>  I don't think there's much point in testing clang-format here, though we should have one test that verifies we're calling it at all.)

Personally, I prefer not doing format in tests -- it would make writing testcases easier, it is annoying to spend time on figuring out **format-only** differences between actual code and expected code.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739