Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
40 msx64 debian > LLVM.CodeGen/AArch64/GlobalISel::irtranslator-inline-asm.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-darwin-ios13 -O0 -global-isel -stop-after=irtranslator -verify-machineinstrs -o - /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
90 msx64 debian > LLVM.CodeGen/AMDGPU/GlobalISel::irtranslator-inline-asm.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 -O0 -global-isel -stop-after=irtranslator -verify-machineinstrs -o - /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll
70 msx64 debian > LLVM.CodeGen/X86::callbr-asm-kill.mir
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -O2 -run-pass=livevars,phi-node-elimination -o - /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/callbr-asm-kill.mir | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/callbr-asm-kill.mir

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.

3902

Please resort.

clang/unittests/Format/FormatTest.cpp
20254

Please add the parsing test for the enum.

25070–25075

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.Thu, Sep 29, 12:38 AM
rymiel added a reviewer: eoanermine.
rymiel added a subscriber: rymiel.
rymiel updated this revision to Diff 463772.Thu, Sep 29, 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.Thu, Sep 29, 12:42 AM
rymiel updated this revision to Diff 463773.Thu, Sep 29, 12:44 AM

Missed a spot when re-sorting