Febbe marked 2 inline comments as done.
Febbe added inline comments.
================
Comment at: clang/lib/Format/Format.cpp:1379
LLVMStyle.IncludeStyle.IncludeCategories = {
- {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
- {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
- {".*", 1, 0, false}};
+ {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 2, false},
+ {"^(<|\"(gtest|gmock|isl|json)/)", 3, 3, false},
----------------
HazardyKnusperkeks wrote:
> Febbe wrote:
> > HazardyKnusperkeks wrote:
> > > I don't follow why this is necessary. And I don't feel comfortable with
> > > it.
> > This **was** required, but is not required necessarily, my last update of
> > the patch made this change most likely obsolete.
> >
> > But the actual reason is, that `SortPriority` is described in the
> > documentation as "optional value which is defaulted to `Priority`, if
> > absent", but it is neither a (std::)optional value nor it was defaulted to
> > Priority if absent.
> > It was indirectly updated from **0** to `Priority` in the algorithm.
> >
> > Since I moved the buggy, re-initialization of `SortPriority` to the first
> > initialization:
> > clang/lib/Tooling/Inclusions/IncludeStyle.cpp:L20 this change must also be
> > covered by the tests: All zero initializations must now be default to
> > `Priority` to not change the test behavior, which is what you want.
> >
> > Either by creating a Constructor without `SortPriority`, which defaults to
> > Priority,
> > creating a factory function, or by doing this for all tests manually.
> >
> >
> > Imagine the tests would also test the interpretation of the JSON input.
> > Then the tests would not require an adjustment to be semantically equal.
> >
> > But since I added Priority to the sorting after **this** change, this is
> > kind of irrelevant.
> My main question is, does this change behavior? In either case, please add
> some tests to show the difference, or the lack thereof.
I removed all changes to the tests, which do not change the behavior.
One test required a change. It assumed, that only one file can be a main
include, which is, in my opinion, not correct. I've made the change, that all
files, which are matched as main include are ordered with priority 0 (this
especially means includes introduced by `IncludeIsMainRegex` and
`IncludeIsMainSourceRegex`). The previous implementation ignored those, if
another main file was found earlier. It could also result in unpredictable
resorting.
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
