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

Mon Feb 11 02:02:21 PST 2019

sammccall added a comment.

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.

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

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57739