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

Mon Apr 17 02:32:50 PDT 2023

Febbe added a comment.

In D143870#4273075 <https://reviews.llvm.org/D143870#4273075>, @MyDeveloperDay wrote:

> I know it might not seem an obvious use case but there really isn't a requirement to not include header files more than once.. imagine if I have
>
>   #define ARCH "win32"
>   #include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"
>   #define ARCH "win64"
>   #include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"
>
> Its not nice, but just because I include it twice doesn't mean its wrong? This change would break code written that way, No?

Mhh, I need to test that. But you are right, if it removes the header in one of the conditional preprocessor blocks, it would be very bad.
Regarding normal blocks, the behavior was already inconsistent, as soon to reorder blocks was activated:
Blocks are merged if possible, then de-duplicated and split again. So I just made the behavior more consistent here.

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. 
Also, when a behavior is now undesirable, it should be possible to adapt them.
I would say in this case I should replace the test with your case, to reflect that includes must not be removed, when they are in different conditional preprocessor blocks.
That the different "modi" should be more consistent is in my opinion desirable.

Last but not least, I currently have nearly zero free time, so I would like to pause this and my other phab until my thesis is done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143870