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

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

Febbe added a comment.

I've added some elaborations and justifications for the criticized changes.



================
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:
> 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.


================
Comment at: clang/lib/Tooling/Inclusions/IncludeStyle.cpp:18
     IO &IO, IncludeStyle::IncludeCategory &Category) {
-  IO.mapOptional("Regex", Category.Regex);
-  IO.mapOptional("Priority", Category.Priority);
-  IO.mapOptional("SortPriority", Category.SortPriority);
+  IO.mapRequired("Regex", Category.Regex);
+  IO.mapRequired("Priority", Category.Priority);
----------------
HazardyKnusperkeks wrote:
> A big no! You will break existing configurations.
Can you elaborate?
Do you mean the change from `mapOptional` to `mapRequired`?
>From what I thought, this change only affects clarity (improvement), not the semantic:
 - the parent JSON list/array entry is already optional.
 - therefore any of them is required, but none is useful alone.
   - an empty regex, just matches nothing `=>` regex required
   - an empty Priority should default (documentation) to INT_MAX, but actually did not (0). 
 - Also, from the documentations view, omitting one of them was never intended.

The only effect this change will have for old configurations, is, that it breaks "wrong" configurations. It's like abusing undefined behav.: You can't be sure it works after a compiler update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143691