[clang-format] Extend SpaceBeforeParens for requires

[HazardyKnusperkeks]

Summary

We can now configure the space between requires and the following paren,separate for clauses and expressions.

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

3999

You meant "if there is one", right?

clang/unittests/Format/FormatTest.cpp

15000

I'd like to see some tests for mixed true/false, false/true values of these parameters. You only test false/false and true/true combinations so far.

23894

This test seems redundant. Does it test something else than the above added ones?

[HazardyKnusperkeks]

Addressed comments.

clang/docs/ClangFormatStyleOptions.rst

3999

Yeah

clang/unittests/Format/FormatTest.cpp

23894

No it doesn't, I just thought I would put the requires stuff into the requires test case. I for one work with the gtest filter on single test cases if I work on some issue.

[HazardyKnusperkeks]

Now the new diff.

[MyDeveloperDay]

LGTM (nit the clang-format check)

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3255–3258

Nit: remove else.

[HazardyKnusperkeks]

Delayed until D113319 is resolved.

[• Quuxplusone]

clang/docs/ClangFormatStyleOptions.rst

3999

IMO these options should be named InRequiresClause and InRequiresExpression: that's where the space is going. The space doesn't go after the requires-clause. The space doesn't go after the requires-expression.
It occurs to me that the name of this option could be analogous to the name of the option that controls []() {} versus [] () {}... except that it looks like there is no such option (and I'm happy about that). :) Also, the name of that option would probably just be AfterCaptureList, which doesn't help us in this case.

clang/include/clang/Format/Format.h

3590

Here and line 3380, and possibly elsewhere: s/parentheses/parenthesis/

clang/unittests/Format/FormatTest.cpp

15063

Throughout, I'd like to see simpler test cases and more of them. E.g. I think this would suffice, in lieu of lines 14305–14369:

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = true;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = true;
verifyFormat("void f(auto x) requires (requires (int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires (int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires (int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = true;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = false;
verifyFormat("void f(auto x) requires (requires(int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires(int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires(int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = false;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = true;
verifyFormat("void f(auto x) requires(requires (int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires (int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires (int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = false;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = false;
verifyFormat("void f(auto x) requires(requires(int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires(int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires(int i) { x+i; };", SpaceAfterRequires);

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

3999

I get your point, but the space does not go anywhere in the clause/expression, so AfterRequiresForClauses?

[• Quuxplusone]

clang/docs/ClangFormatStyleOptions.rst

3999

(I'd avoid the word For, because of keyword for.)
A requires-expression is the whole expression, requires + param-list + body: https://eel.is/c++draft/expr.prim.req#nt:requires-expression
A requires-clause is the whole clause, requires + logical-or-expr: https://eel.is/c++draft/temp.pre#nt:requires-clause
Does that resolve your concern about the word In?

[HazardyKnusperkeks]

• Rebased
• Updated
• Tests changed

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

3999

My concern is: In may refer to anywhere in the clause, but we are talking about one very specific point per clause. (Analogues for expression.)

The current version is very explicit, and I don't have a problem with such a long name.

clang/include/clang/Format/Format.h

3590

Non native here, but I think one could argue both were okay. I just copied what the other options state.

[• Quuxplusone]

clang/include/clang/Format/Format.h

3590

It's definitely not grammatical to talk about "opening parentheses, if there is one". But if this is just copying pre-existing wording, then OK, no objection to someone handling the grammar in a followup PR.

[HazardyKnusperkeks]

A friendly ping.

[HazardyKnusperkeks]

I need a response here. :) @Quuxplusone are the names okay for you?

[curdeius]

LGTM. My naming suggestion is not binding. I have no strong opinion on this, but a shorter name would get my 👍 :).
So please sync with other reviewers.

clang/docs/ClangFormatStyleOptions.rst

4007

Wouldn't AfterRequiresInClause/AfterRequiresInExpression be meaningful enough?

[• Quuxplusone]

clang/docs/ClangFormatStyleOptions.rst

4007

+1, I have no problem with AfterRequiresInClause and AfterRequiresInExpression!

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

4007

I will change the names accordingly before the commit.

They are much better.

clang/docs/ClangFormatStyleOptions.rst

* ``bool AfterIfMacros`` If ``true``, put space between if macros and opening parentheses.
 
     .. code-block:: c++
 
        true:                                  false:
        IF (...)                        vs.    IF(...)
          <conditional-body>                     <conditional-body>
 
   * ``bool AfterOverloadedOperator`` If ``true``, put a space between operator overloading and opening
     parentheses.
 
     .. code-block:: c++
 
        true:                                  false:
        void operator++ (int a);        vs.    void operator++(int a);
        object.operator++ (10);                object.operator++(10);
 
+  * ``bool AfterRequiresInClause`` If ``true``, put space between requires keyword in a requires clause and
+    opening parentheses, if there is one.
+
+    .. code-block:: c++
+
+       true:                                  false:
+       template<typename T>            vs.    template<typename T>
+       requires (A<T> && B<T>)                requires(A<T> && B<T>)
+       ...                                    ...
+
+  * ``bool AfterRequiresInExpression`` If ``true``, put space between requires keyword in a requires expression
+    and opening parentheses.
+
+    .. code-block:: c++
+
+       true:                                  false:
+       template<typename T>            vs.    template<typename T>
+       concept C = requires (T t) {           concept C = requires(T t) {
+                     ...                                    ...
+                   }                                      }
+
   * ``bool BeforeNonEmptyParentheses`` If ``true``, put a space before opening parentheses only if the
     parentheses are not empty.
 
     .. code-block:: c++
 
        true:                                  false:
        void f (int a);                 vs.    void f();
        f (a);                                 f();

clang/include/clang/Format/Format.h

struct SpaceBeforeParensCustom {
     /// If ``true``, put space between if macros and opening parentheses.
     /// \code
     ///    true:                                  false:
     ///    IF (...)                        vs.    IF(...)
     ///      <conditional-body>                     <conditional-body>
     /// \endcode
     bool AfterIfMacros;
     /// If ``true``, put a space between operator overloading and opening
     /// parentheses.
     /// \code
     ///    true:                                  false:
     ///    void operator++ (int a);        vs.    void operator++(int a);
     ///    object.operator++ (10);                object.operator++(10);
     /// \endcode
     bool AfterOverloadedOperator;
+    /// If ``true``, put space between requires keyword in a requires clause and
+    /// opening parentheses, if there is one.
+    /// \code
+    ///    true:                                  false:
+    ///    template<typename T>            vs.    template<typename T>
+    ///    requires (A<T> && B<T>)                requires(A<T> && B<T>)
+    ///    ...                                    ...
+    /// \endcode
+    bool AfterRequiresInClause;
+    /// If ``true``, put space between requires keyword in a requires expression
+    /// and opening parentheses.
+    /// \code
+    ///    true:                                  false:
+    ///    template<typename T>            vs.    template<typename T>
+    ///    concept C = requires (T t) {           concept C = requires(T t) {
+    ///                  ...                                    ...
+    ///                }                                      }
+    /// \endcode
+    bool AfterRequiresInExpression;
     /// If ``true``, put a space before opening parentheses only if the
     /// parentheses are not empty.
     /// \code
     ///    true:                                  false:
     ///    void f (int a);                 vs.    void f();
     ///    f (a);                                 f();
     /// \endcode
     bool BeforeNonEmptyParentheses;
 
     SpaceBeforeParensCustom()
         : AfterControlStatements(false), AfterForeachMacros(false),
           AfterFunctionDeclarationName(false),
           AfterFunctionDefinitionName(false), AfterIfMacros(false),
-          AfterOverloadedOperator(false), BeforeNonEmptyParentheses(false) {}
+          AfterOverloadedOperator(false), AfterRequiresInClause(false),
+          AfterRequiresInExpression(false), BeforeNonEmptyParentheses(false) {}
 
     bool operator==(const SpaceBeforeParensCustom &Other) const {
       return AfterControlStatements == Other.AfterControlStatements &&
              AfterForeachMacros == Other.AfterForeachMacros &&
              AfterFunctionDeclarationName ==
                  Other.AfterFunctionDeclarationName &&
              AfterFunctionDefinitionName == Other.AfterFunctionDefinitionName &&
              AfterIfMacros == Other.AfterIfMacros &&
              AfterOverloadedOperator == Other.AfterOverloadedOperator &&
+             AfterRequiresInClause == Other.AfterRequiresInClause &&
+             AfterRequiresInExpression == Other.AfterRequiresInExpression &&
              BeforeNonEmptyParentheses == Other.BeforeNonEmptyParentheses;
     }
   };
 
   /// Control of individual space before parentheses.
   ///
   /// If ``SpaceBeforeParens`` is set to ``Custom``, use this to specify
   /// how each individual space before parentheses case should be handled.

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle::SpaceBeforeParensCustom &Spacing) {
     IO.mapOptional("AfterControlStatements", Spacing.AfterControlStatements);
     IO.mapOptional("AfterForeachMacros", Spacing.AfterForeachMacros);
     IO.mapOptional("AfterFunctionDefinitionName",
                    Spacing.AfterFunctionDefinitionName);
     IO.mapOptional("AfterFunctionDeclarationName",
                    Spacing.AfterFunctionDeclarationName);
     IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
     IO.mapOptional("AfterOverloadedOperator", Spacing.AfterOverloadedOperator);
+    IO.mapOptional("AfterRequiresInClause", Spacing.AfterRequiresInClause);
+    IO.mapOptional("AfterRequiresInExpression",
+                   Spacing.AfterRequiresInExpression);
     IO.mapOptional("BeforeNonEmptyParentheses",
                    Spacing.BeforeNonEmptyParentheses);
   }
 };
 
 template <> struct MappingTraits<FormatStyle::RawStringFormat> {
   static void mapping(IO &IO, FormatStyle::RawStringFormat &Format) {
     IO.mapOptional("Language", Format.Language);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
   LLVMStyle.SpaceBeforeCaseColon = false;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
+  LLVMStyle.SpaceBeforeParensOptions = {};
   LLVMStyle.SpaceBeforeParensOptions.AfterControlStatements = true;
   LLVMStyle.SpaceBeforeParensOptions.AfterForeachMacros = true;
   LLVMStyle.SpaceBeforeParensOptions.AfterIfMacros = true;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   // Space between template and attribute.
   // e.g. template <typename T> [[nodiscard]] ...
   if (Left.is(TT_TemplateCloser) && Right.is(TT_AttributeSquare))
     return true;
   // Space before parentheses common for all languages
   if (Right.is(tok::l_paren)) {
     if (Left.is(TT_TemplateCloser) && Right.isNot(TT_FunctionTypeLParen))
       return spaceRequiredBeforeParens(Right);
-    if (Left.is(tok::kw_requires))
-      return spaceRequiredBeforeParens(Right);
+    if (Left.isOneOf(TT_RequiresClause, TT_RequiresClauseInARequiresExpression))
+      return Style.SpaceBeforeParensOptions.AfterRequiresInClause ||
+             spaceRequiredBeforeParens(Right);
+    if (Left.is(TT_RequiresExpression))
+      return Style.SpaceBeforeParensOptions.AfterRequiresInExpression ||
+             spaceRequiredBeforeParens(Right);
     if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
         (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
       return true;
     if (Left.is(TT_ForEachMacro))
       return Style.SpaceBeforeParensOptions.AfterForeachMacros ||
              spaceRequiredBeforeParens(Right);
     if (Left.is(TT_IfMacro))
       return Style.SpaceBeforeParensOptions.AfterIfMacros ||
              spaceRequiredBeforeParens(Right);
     if (Line.Type == LT_ObjCDecl)
       return true;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("typedef void (*cb) (int);", SomeSpace2);
   verifyFormat("T A::operator()();", SomeSpace2);
   // verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
 
   FormatStyle SpaceAfterOverloadedOperator = getLLVMStyle();
   SpaceAfterOverloadedOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
   SpaceAfterOverloadedOperator.SpaceBeforeParensOptions
       .AfterOverloadedOperator = true;
 
   verifyFormat("auto operator++ () -> int;", SpaceAfterOverloadedOperator);
   verifyFormat("X A::operator++ ();", SpaceAfterOverloadedOperator);
   verifyFormat("some_object.operator++ ();", SpaceAfterOverloadedOperator);
   verifyFormat("auto func() -> int;", SpaceAfterOverloadedOperator);
 
   SpaceAfterOverloadedOperator.SpaceBeforeParensOptions
       .AfterOverloadedOperator = false;
 
   verifyFormat("auto operator++() -> int;", SpaceAfterOverloadedOperator);
   verifyFormat("X A::operator++();", SpaceAfterOverloadedOperator);
   verifyFormat("some_object.operator++();", SpaceAfterOverloadedOperator);
   verifyFormat("auto func() -> int;", SpaceAfterOverloadedOperator);
+
+  auto SpaceAfterRequires = getLLVMStyle();
+  SpaceAfterRequires.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_FALSE(
+      SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInClause);
+  EXPECT_FALSE(
+      SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInExpression);
+  verifyFormat("void f(auto x)\n"
+               "  requires requires(int i) { x + i; }\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("void f(auto x)\n"
+               "  requires(requires(int i) { x + i; })\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("if (requires(int i) { x + i; })\n"
+               "  return;",
+               SpaceAfterRequires);
+  verifyFormat("bool b = requires(int i) { x + i; };", SpaceAfterRequires);
+  verifyFormat("template <typename T>\n"
+               "  requires(Foo<T>)\n"
+               "class Bar;",
+               SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInClause = true;
+  verifyFormat("void f(auto x)\n"
+               "  requires requires(int i) { x + i; }\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("void f(auto x)\n"
+               "  requires (requires(int i) { x + i; })\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("if (requires(int i) { x + i; })\n"
+               "  return;",
+               SpaceAfterRequires);
+  verifyFormat("bool b = requires(int i) { x + i; };", SpaceAfterRequires);
+  verifyFormat("template <typename T>\n"
+               "  requires (Foo<T>)\n"
+               "class Bar;",
+               SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInClause = false;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInExpression = true;
+  verifyFormat("void f(auto x)\n"
+               "  requires requires (int i) { x + i; }\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("void f(auto x)\n"
+               "  requires(requires (int i) { x + i; })\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("if (requires (int i) { x + i; })\n"
+               "  return;",
+               SpaceAfterRequires);
+  verifyFormat("bool b = requires (int i) { x + i; };", SpaceAfterRequires);
+  verifyFormat("template <typename T>\n"
+               "  requires(Foo<T>)\n"
+               "class Bar;",
+               SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresInClause = true;
+  verifyFormat("void f(auto x)\n"
+               "  requires requires (int i) { x + i; }\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("void f(auto x)\n"
+               "  requires (requires (int i) { x + i; })\n"
+               "{}",
+               SpaceAfterRequires);
+  verifyFormat("if (requires (int i) { x + i; })\n"
+               "  return;",
+               SpaceAfterRequires);
+  verifyFormat("bool b = requires (int i) { x + i; };", SpaceAfterRequires);
+  verifyFormat("template <typename T>\n"
+               "  requires (Foo<T>)\n"
+               "class Bar;",
+               SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpaceAfterLogicalNot = true;
 
   verifyFormat("bool x = ! y", Spaces);
   verifyFormat("if (! isFailure())", Spaces);
verifyFormat("template <typename T>\n"
                "  requires Foo<T> void bar() {}\n"
                "template <typename T>\n"
                "void bar()\n"
                "  requires Foo<T> {}\n"
                "template <typename T>\n"
                "  requires Foo<T> Bar(T) -> Bar<T>;",
                Style);
 
   verifyFormat("template <typename AAAAAAA>\n"
                "  requires Foo<AAAAAAAAAAAAAAAA>\n"
                "struct Bar {};\n"
                "template <typename AAAAAAA>\n"
                "  requires Foo<AAAAAAAAAAAAAAAA>\n"
                "void bar() {}\n"
                "template <typename AAAAAAA>\n"
                "void bar()\n"
                "  requires Foo<AAAAAAAAAAAAAAAA> {}\n"