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