This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add option for aligning requires clause body
ClosedPublic

Authored by rymiel on Jul 10 2022, 5:42 AM.

Details

Summary
  • Add an option whether requires clause body should be aligned with requires keyword
  • Add tests for RequiresExpressionIndentation

Fixes https://github.com/llvm/llvm-project/issues/56283

Diff Detail

Event Timeline

eoanermine created this revision.Jul 10 2022, 5:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2022, 5:42 AM
eoanermine requested review of this revision.Jul 10 2022, 5:42 AM
curdeius requested changes to this revision.Jul 10 2022, 10:15 PM

Haven't you forgotten to add formatting tests? :)

This revision now requires changes to proceed.Jul 10 2022, 10:15 PM
HazardyKnusperkeks removed a reviewer: Restricted Project.Jul 11 2022, 12:36 PM

Thanks for the patch. Please add the full context for the next revision: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Haven't you forgotten to add formatting tests? :)

Yes, I have. Thank you.

eoanermine updated this revision to Diff 443895.EditedJul 12 2022, 3:04 AM

Add tests for AlignRequiresClauseBody option
Add full context

Add tests for AlignRequiresClauseBody option

Are these tests fine?

Vigilans added a comment.EditedJul 12 2022, 9:59 AM

In my knowledge of clang-format, Requires Clause and Requires Expression are treated differently.
In cppreference, A Requires Clause is specific to the requires keyword used for constraints on template arguments or on a function declaration. In this issue's context of continuation indent, the requires keyword is used for Requires Expression, just as the code relative to this diff suggests:

if (Current.is(TT_RequiresExpression) /* <-- it's requires expression here */ && Style.AlignRequiresClauseBody)
  CurrentState.NestedBlockIndent = State.Column;

Some more example options in clang-format:

  • IndentRequiresClause
true:
template <typename It>
  requires Iterator<It>
void sort(It begin, It end) {
  //....
}

false:
template <typename It>
requires Iterator<It>
void sort(It begin, It end) {
  //....
}
  • SpaceBeforeParensOptions -> AfterRequiresInClause
true:                                  false:
template<typename T>            vs.    template<typename T>
requires (A<T> && B<T>)                requires(A<T> && B<T>)
...                                    ...
  • SpaceBeforeParensOptions -> AfterRequiresInExpression
true:                                  false:
template<typename T>            vs.    template<typename T>
concept C = requires (T t) {           concept C = requires(T t) {
              ...                                    ...
            }                                      }

So the option in this diff should use RequiresExpressionBody.

Besides, there is an option that does similar thing as this diff do:

  • LambdaBodyIndentation

The indentation style of lambda bodies. Signature (the default) causes the lambda body to be indented one additional level relative to the indentation level of the signature. OuterScope forces the lambda body to be indented one additional level relative to the parent scope containing the lambda signature. For callback-heavy code, it may improve readability to have the signature indented two levels and to use OuterScope. The KJ style guide requires OuterScope. KJ style guide

Possible values:

  • LBI_Signature (in configuration: Signature) Align lambda body relative to the lambda signature. This is the default.
someMethod(
    [](SomeReallyLongLambdaSignatureArgument foo) {
      return;
    });
  • LBI_OuterScope (in configuration: OuterScope) Align lambda body relative to the indentation level of the outer scope the lambda signature resides in.
someMethod(
    [](SomeReallyLongLambdaSignatureArgument foo) {
  return;
});

Related code of implementation: https://github.com/llvm/llvm-project/blob/f338f416baf09aaed59b1b3bc693cab5d5cdf7ab/clang/lib/Format/UnwrappedLineFormatter.cpp#L967-L978

So we may select a similar option naming (e.g. RequiresExpressionIndentation) and values (e.g. RequiresExpressionIndentationKind with Keyword and OuterScope) as this lambda body option for consistency, and leaving some room for future update? (I have encountered some more cases about concept definitions that may expect requires expression with more intelligent indentation style, though left behind and conforming with current clang-format style.)

In my knowledge of clang-format, Requires Clause and Requires Expression are treated differently.
<snip>

+1 for the enum, even if we only have 2 options yet.

And yes, the option is wrongly named. You want to indent/align requires expressions, not clauses.

The tests seem mostly to suffice. You could add a test where the outer scope is indented, e.g. a class with a member function, and a test where the expression is used within an if.

Please also run clang/docs/tools/dump_format_style.py to update the ClangFormatStyleOptions.rst.

HazardyKnusperkeks requested changes to this revision.Jul 12 2022, 2:32 PM
This revision now requires changes to proceed.Jul 12 2022, 2:32 PM
eoanermine edited the summary of this revision. (Show Details)
  • Rename AlignRequiresClauseBody to RequiresExpressionIndentation
  • Introduce RequiresExpressionIndentationKind as a type of RequiresExpressionIndentation
  • Add more tests

Looks very promising.

clang/include/clang/Format/Format.h
396

Please resort.

3885

Please resort.

clang/unittests/Format/FormatTest.cpp
20036

Please add the parsing test for the enum.

24805–24810

This is a bad example, since we can't see any difference.

What happens if it's not the first expression in the if.

rymiel commandeered this revision.Sep 29 2022, 12:38 AM
rymiel added a reviewer: eoanermine.
rymiel added a subscriber: rymiel.
rymiel updated this revision to Diff 463772.Sep 29 2022, 12:42 AM

Resolve nits:

  • Re-sorted uses of the name RequiresExpressionIndentationKind
  • Updated version to be 16
  • Add test for demonstrating indentation as subexpression
  • Reformatted
rymiel marked 3 inline comments as done.Sep 29 2022, 12:42 AM
rymiel updated this revision to Diff 463773.Sep 29 2022, 12:44 AM

Missed a spot when re-sorting

Is this option really required? Can we just fix the regression as reported in https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't followed the policy lately when adding new options.

rymiel added a comment.Oct 7 2022, 2:00 AM

It wasn't an accidental regression, though, it was a conscious formatting decision (maybe ask @HazardyKnusperkeks for more). Similar to how lambdas can be formatted aligned to the introducer, i think this option does make sense.

Is this option really required? Can we just fix the regression as reported in https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't followed the policy lately when adding new options.

If it's a regression or not is a matter of perspective. Before my patch there were no real handling of concepts and they just looked like that. I had changed that (on purpose) if I remember correctly to bring in closer to lambdas.
We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Also asking for a style guide concerning concepts and requires is a bit hard I think, I think in none of the styles we implement it is mentioned, it at least wasn't when I added the handling.

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

  1. Only fix the "regression".
  2. Add the option and make OuterScope the default.
  3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

clang/unittests/Format/FormatTest.cpp
20036

@rymiel Is this done?

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

  1. Only fix the "regression".
  2. Add the option and make OuterScope the default.
  3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

Add the option, and then either way we will have a regression to somebody. OuterScope will regress to version 15. Keyword up to 14. So it should be the most reasonable to make OuterScope the default, since the early adapters will most be more open to change their configuration.

@rymiel please add a change log note about that changed default.

Vigilans accepted this revision.Oct 8 2022, 5:26 AM

We can fix that regression, and I will adopt this change for my company to keep the requires expressions where they are. :)

Then we have three options:

  1. Only fix the "regression".
  2. Add the option and make OuterScope the default.
  3. Add the option and make Keyword the default.

@HazardyKnusperkeks I will defer to your choice. :)

Add the option, and then either way we will have a regression to somebody. OuterScope will regress to version 15. Keyword up to 14. So it should be the most reasonable to make OuterScope the default, since the early adapters will most be more open to change their configuration.

@rymiel please add a change log note about that changed default.

I am fine with either default option with their persuasive reasons:

  1. Default to Keyword: Interpreted as Formally documented formatting behavior, and the options arrangement looks more consistent with LambdaBodyIndentation (Signature (default) and OuterScope).
  2. Default to OuterScope: Interpreted as Fix the regression, and the style looks more consistent with example codes in common concept tutorials (the expression is indented the same level as concept keyword, not requires keyword).

For consistency with clang-format current behaviors and options, we may prefer Keyword; For consistency with common current tutorial code, we may prefer OuterScope.

rymiel updated this revision to Diff 466551.Oct 10 2022, 10:50 AM

Flip the default and note that in the Release Notes

Sorry, I did not realize I was mentioned.
I think the default option will always be the first option? So I swapped them around.
Note that I didn't change the LLVM config, which right now is still set to Keyword.
I'm also not sure if the release notes should note this as "important" or "breaking"

rymiel marked 2 inline comments as done.Oct 11 2022, 10:52 AM
rymiel added inline comments.
clang/unittests/Format/FormatTest.cpp
20036

The code this comment was added on has been completely removed; so i guess i'll mark it as done

rymiel marked an inline comment as done.Oct 11 2022, 10:53 AM

Note that I didn't change the LLVM config, which right now is still set to Keyword.

We should change it back to OuterScope.

clang/lib/Format/Format.cpp
1304

Delete it.

clang/docs/ReleaseNotes.rst
540 ↗(On Diff #466551)

Unrelated.

clang/lib/Format/Format.cpp
1304

Why delete?
Set it to OuterScope.

owenpan added inline comments.Oct 12 2022, 8:05 PM
clang/docs/ReleaseNotes.rst
540 ↗(On Diff #466551)

Unrelated.

I’m ok with fixing it here so that we don’t need another patch.

clang/lib/Format/Format.cpp
1304

Why delete?
Set it to OuterScope.

Cuz OuterScope is the default. :)

clang/lib/Format/Format.cpp
1304

The default is uninitialized. Not setting anything here is undefined behavior.

owenpan added inline comments.Oct 13 2022, 3:54 PM
clang/lib/Format/Format.cpp
1304

@HazardyKnusperkeks you are right!

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

I prefer the latter so as to minimize the diff.

HazardyKnusperkeks added a comment.EditedOct 14 2022, 9:16 AM

Changing the default LLVM style would change the output of all the requires-related test cases in FormatTest.Concepts. Should I change these test cases to use the new indentation or pass the REI_Keyword style to them?

I prefer the latter so as to minimize the diff.

+1

Maybe with a comment, why it isn't tested with the LLVM default.

rymiel updated this revision to Diff 468414.Oct 17 2022, 10:07 PM

Swap the default for LLVM style, and adjust tests which depended on the previous style to still use it.

rymiel marked 5 inline comments as done.Oct 17 2022, 10:08 PM
owenpan accepted this revision.Oct 17 2022, 10:49 PM

How should I proceed with a stale rejecting review?

How should I proceed with a stale rejecting review?

You can commit as https://reviews.llvm.org/D129443#3641571 has been addressed.

How should I proceed with a stale rejecting review?

You can commit as https://reviews.llvm.org/D129443#3641571 has been addressed.

And given that he wasn't active in the last days (weeks?), he will probably not respond anytime soon.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 21 2022, 12:53 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.