[PATCH] D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line

Fri Oct 11 14:22:45 PDT 2019

poelmanc added a comment.

In D68682#1705866 <https://reviews.llvm.org/D68682#1705866>, @gribozavr wrote:

> > I guess here's the high-level question: should all removals that remove all non-blank text from a line also delete the line?
>
> I see your point, but as https://llvm.org/PR43583 shows, we have a much larger problem: textual replacements don't compose. So, whatever we do, it will only be hacky workarounds for specific high-value cases.
>
> About how exactly to do it. It would be preferred if checkers that already know that they are deleting a full line, just deleted the newline characters. However, for other cases, I feel like the place where this patch implements newline deletion is not ideal. It implements newline deletion while applying all fixes. Applying fixes is not the only client of fixit information. Probably it is one of the least-important ones, to be honest. I don't know who will run clang-tidy and just trust it to apply whatever fixes it wants. More likely, the user will want to see a preview of findings and fixes, and fixit application will be done by some other tool. Therefore, implementing newline deletion in the code that applies all fixes is not going to be helpful for most workflows.
>
> Therefore, I think the most appropriate place to do this cleanup is `cleanupAroundReplacements`. What do you think?

Thanks, from the name that sounds like the perfect place to do it. If `cleanupAroundReplacements` also is used by clang-format, would we want to make the functionality optional, e.g. via a new bool parameter to `cleanupAroundReplacements`, a new option in FormatStyle, etc.?

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D68682