[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

Mon Apr 17 05:26:17 PDT 2023

MyDeveloperDay added a comment.

> Regarding the comment that I must not change existing tests: I think this rule is too strict, because those tests are mostly regression tests. 
> But a regression tests does not test for correctness. So if a test had already a wrong assumption, it must be changeable.

I don't agree, you are making the assumption that the default behaviour should now be different from what is was before, we have 100,000+ of users did you canvas them to ask if we wanted your suggested correct behaviour? (especially when someone has split their includes into a separate include group? I assume for a reason)

What would be their recourse if they didn't want the duplicate removed across groups, I don't see one? are you expecting them to // clang-format on/off

Ultimately we try not to change the default behaviour, if we think the behaviour is useful (and this could be), we ask that its put behind a new "Option" and that by default its off.

`SortIncludes` was latterly considered a contentious addition and has been proved to alter code in a well publicized example, in hindsight most people think it should have been off by default. I don't like making assumptions that we should turn something on, that others might not want/need or desire. This feels like one of those and something that could break code.

I personally follow a Beyoncé rule when it comes to unit tests... "If I like it I should have put a test on it", if the test is there, I need to be persuaded the test is very wrong before I'm happy to let it be changed to a new behaviour.

FWIW, these are not just regression tests, they assert that the behaviour is as the author desired and that is considered correctness until proven otherwise, any new author needs to respect that, or prove its a genuine bug and not just a matter of style/opinion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143870