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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks 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"
----------------
Febbe wrote:
> 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.
> 
> 
It is very unfortunate that `llvm/a.h` would be detected as a main header, but would currently the relative order be kept and thus `a.h` always be //the// main header?

I don't think you will find someone (of the usual reviewers) to be in favor of the pragma, and I certainly don't want such a thing in my code.

A heuristic seems to be a good way. I'd say from all the candidates we take the one without any directories in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691