[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:
> 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.
> but would currently the relative order be kept and thus a.h always be the main header?

No, unfortunately, clang-format never sorted the path after its dept, std::filesystem::path would do that, but clang-format uses the std::less operator of StringRef which is a lexicographical comparison.
Therefore, `m.h` will always be ordered after `llvm/m.h`. Changing that would reorder nearly all headers in all projects using clang-format. But I could do this only for the main-include candidates.

Nevertheless, I don't think this would be a good heuristic either, because me, and many others are using an extra `include` folder for public headers (API) and the `src` directory for private headers:

```
my-lib/
├─ include/
│  ├─ public_headers.md
│  ├─ a.h
├─ src/
│  ├─ private_headers_and_source.md
│  ├─ a_priv.h
│  ├─ a.cpp
```

With your proposed solution, the primary main header is ordered after the main include.
Let me implement my heuristic first, maybe it will work well enough to prevent the reordering, when the main header is sorted by hand (just like before, but now it also should work for negative priorities).

Otherwise, we must reconsider which tradeoff we choose (maybe a completely new one).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691