HazardyKnusperkeks 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},
----------------
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.
================
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);
----------------
Febbe wrote:
> 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.
> 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
While a configuration without a regex may be stupid, it currently is accepted.
> - an empty Priority should default (documentation) to INT_MAX, but
> actually did not (0).
Here one has to decide if the documentation or the behavior has to be fixed. I
have not a preference, @MyDeveloperDay
may have.
> - Also, from the documentations view, omitting one of them was never
> intended.
Nevertheless it is accepted and thus there is most likely a configuration which
uses it.
> 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.
Changing what happens in such cases is one thing (but also not desired), just
completely turning clang-format off, through erroring on the configuration
parsing is in my view not acceptable.
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
