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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
