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

[Björn Schäpers via Phabricator via cfe-commits]

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