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

[Fabian Keßler via Phabricator via cfe-commits]

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