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