[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added inline comments.


================
Comment at: clang/unittests/Format/SortIncludesTest.cpp:548
+  EXPECT_EQ("#include \"a.h\"\n"
+            "#include \"llvm/a.h\"\n"
+            "#include \"b.h\"\n"
----------------
HazardyKnusperkeks wrote:
> While I may accept there are more than one //main// header, this should not happen in my opinion.
Yes, that is a conflict of interests. There is no perfect way of implementing this, without the help of clang-tidy / the clang language server:

To arguably define a file as a main include, all declarations must be analyzed, whether they are predeclared in the include or not.
But we don't have the help of the clang AST.

My expectation is, that when I randomly shuffle the includes, that they are always sorted to the same result.
Currently, this is not the case, and depending on the rules it can furthermore happen, that a main include "a.h" is exchanged with the "llvm/a.h". 
This should also not happen, but it does, and it is not covered by the tests.

I consider the false negative of a correct main include worse than a false positive.
Therefore, I changed it. 
In addition, my approach has the advantage that all includes are sorted in the same way, regardless of the order.

But if you want, we could introduce a new Option like: `enum FindMainIncludes{FMI_First, FMI_All, FMI_Off = 0};`
With `First` to match the current behavior, `All` for the new behavior.
But then matched includes with a negative priority would be swapped with the other possible main_include at each run.
I don't know how to prevent this.
The best solution I can imagine, is still a comment of the programmer, that the respective include is or is not a main include.
E.g. `// clang-format pragma: no_main_include`

Another idea would be, to insert all main includes into a list with the matchers' priority, and then take the last negative or the first positive, but only if it is not partially sortable with the neighbors.
This might be a good heuristic, whether the include was previously detected as main include.
But I don't know if this is implementable with the current algorithm.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691