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