[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb786a4aefeda: [clang-format] Extend SpaceBeforeParens for 
requires (authored by HazardyKnusperkeks).

Changed prior to commit:
  https://reviews.llvm.org/D113369?vs=406450=409013#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15012,6 +15012,84 @@
   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 \n"
+   "  requires(Foo)\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 \n"
+   "  requires (Foo)\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 \n"
+   "  requires(Foo)\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 \n"
+   "  requires (Foo)\n"
+   "class Bar;",
+   SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3247,8 +3247,12 @@
   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 

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks marked 2 inline comments as done.
HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:4007
 
+  * ``bool AfterRequiresKeywordInRequiresClause`` If ``true``, put space 
between requires keyword in a requires clause and
+opening parentheses, if there is one.

Quuxplusone wrote:
> curdeius wrote:
> > Wouldn't `AfterRequiresInClause`/`AfterRequiresInExpression` be meaningful 
> > enough?
> +1, I have no problem with `AfterRequiresInClause` and 
> `AfterRequiresInExpression`!
I will change the names accordingly before the commit.

They are much better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:4007
 
+  * ``bool AfterRequiresKeywordInRequiresClause`` If ``true``, put space 
between requires keyword in a requires clause and
+opening parentheses, if there is one.

curdeius wrote:
> Wouldn't `AfterRequiresInClause`/`AfterRequiresInExpression` be meaningful 
> enough?
+1, I have no problem with `AfterRequiresInClause` and 
`AfterRequiresInExpression`!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang/docs/ClangFormatStyleOptions.rst:4007
 
+  * ``bool AfterRequiresKeywordInRequiresClause`` If ``true``, put space 
between requires keyword in a requires clause and
+opening parentheses, if there is one.

Wouldn't `AfterRequiresInClause`/`AfterRequiresInExpression` be meaningful 
enough?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks added a comment.

A friendly ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/Format/Format.h:3371
+/// If ``true``, put space between requires keyword in a requires clause 
and
+/// opening parentheses, if there is one.
+/// \code

HazardyKnusperkeks wrote:
> Quuxplusone wrote:
> > Here and line 3380, and possibly elsewhere: `s/parentheses/parenthesis/`
> Non native here, but I think one could argue both were okay. I just copied 
> what the other options state.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks requested review of this revision.
HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

Quuxplusone wrote:
> HazardyKnusperkeks wrote:
> > Quuxplusone wrote:
> > > HazardyKnusperkeks wrote:
> > > > curdeius wrote:
> > > > > You meant "if there is one", right?
> > > > Yeah
> > > 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.
> > I get your point, but the space does not go anywhere in the 
> > clause/expression, so `AfterRequiresForClauses`?
> (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`?
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.



Comment at: clang/include/clang/Format/Format.h:3371
+/// If ``true``, put space between requires keyword in a requires clause 
and
+/// opening parentheses, if there is one.
+/// \code

Quuxplusone wrote:
> Here and line 3380, and possibly elsewhere: `s/parentheses/parenthesis/`
Non native here, but I think one could argue both were okay. I just copied what 
the other options state.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks updated this revision to Diff 406450.
HazardyKnusperkeks marked 4 inline comments as done.
HazardyKnusperkeks added a reviewer: curdeius.
HazardyKnusperkeks removed a subscriber: curdeius.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

- Rebased
- Updated
- Tests changed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14912,6 +14912,88 @@
   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
+   .AfterRequiresKeywordInRequiresClause);
+  EXPECT_FALSE(SpaceAfterRequires.SpaceBeforeParensOptions
+   .AfterRequiresKeywordInRequiresExpression);
+  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 \n"
+   "  requires(Foo)\n"
+   "class Bar;",
+   SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions
+  .AfterRequiresKeywordInRequiresClause = 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 \n"
+   "  requires (Foo)\n"
+   "class Bar;",
+   SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions
+  .AfterRequiresKeywordInRequiresClause = false;
+  SpaceAfterRequires.SpaceBeforeParensOptions
+  .AfterRequiresKeywordInRequiresExpression = 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 \n"
+   "  requires(Foo)\n"
+   "class Bar;",
+   SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions
+  .AfterRequiresKeywordInRequiresClause = 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 \n"
+   "  requires (Foo)\n"
+   "class Bar;",
+   SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3211,8 +3211,14 @@
   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 

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

HazardyKnusperkeks wrote:
> Quuxplusone wrote:
> > HazardyKnusperkeks wrote:
> > > curdeius wrote:
> > > > You meant "if there is one", right?
> > > Yeah
> > 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.
> I get your point, but the space does not go anywhere in the 
> clause/expression, so `AfterRequiresForClauses`?
(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`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

Quuxplusone wrote:
> HazardyKnusperkeks wrote:
> > curdeius wrote:
> > > You meant "if there is one", right?
> > Yeah
> 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.
I get your point, but the space does not go anywhere in the clause/expression, 
so `AfterRequiresForClauses`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

HazardyKnusperkeks wrote:
> curdeius wrote:
> > You meant "if there is one", right?
> Yeah
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.



Comment at: clang/include/clang/Format/Format.h:3371
+/// If ``true``, put space between requires keyword in a requires clause 
and
+/// opening parentheses, if there is one.
+/// \code

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



Comment at: clang/unittests/Format/FormatTest.cpp:14369
+  "{}",
+  SpaceAfterRequires);
 }

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);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks planned changes to this revision.
HazardyKnusperkeks marked an inline comment as done.
HazardyKnusperkeks added a comment.

Delayed until D113319  is resolved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3145-3148
+  else {
+assert(Left.is(TT_RequiresExpression));
+return Style.SpaceBeforeParensOptions.AfterRequiresExpression;
+  }

Nit: remove `else`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM (nit the clang-format check)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks updated this revision to Diff 385594.
HazardyKnusperkeks added a comment.

Now the new diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14300,6 +14300,73 @@
   verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
+
+  auto SpaceAfterRequires = getLLVMStyle();
+  SpaceAfterRequires.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  verifyFormat(
+  "template  void func(T) requires(trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat("template  void func(T) requires requires(T &) {\n"
+   "  typename T::size_type;\n"
+   "  { t.size() } -> std::same_as;\n"
+   "}\n"
+   "{}",
+   SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = false;
+  verifyFormat(
+  "template  void func(T) requires (trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat(
+  "template  void func(T) requires requires(T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = false;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat(
+  "template  void func(T) requires(trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat(
+  "template  void func(T) requires (trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
@@ -22565,6 +22632,21 @@
"}\n"
"{}",
Style);
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  Style.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  Style.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat("template \n"
+   "  requires (std::is_integral_v && std::is_signed_v)\n"
+   "void func(T) {}",
+   Style);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  Style);
 }
 
 TEST_F(FormatTest, StatementAttributeLikeMacros) {
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2477,6 +2477,13 @@
 void UnwrappedLineParser::parseConstraintExpression(
 unsigned int OriginalLevel) {
   // requires Id && Id || Id
+
+  if (FormatTok->Tok.is(tok::kw_requires)) {
+assert(FormatTok->Previous);
+assert(FormatTok->Previous->is(TT_RequiresClause));
+FormatTok->setType(TT_RequiresExpression);
+  }
+
   while (
   FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
 nextToken();
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3137,8 +3137,16 @@
   if (Right.is(tok::l_paren)) {
 if (Left.is(TT_TemplateCloser) && Right.isNot(TT_FunctionTypeLParen))
   return spaceRequiredBeforeParens(Right);
-if (Left.is(tok::kw_requires))
-  

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

curdeius wrote:
> You meant "if there is one", right?
Yeah



Comment at: clang/unittests/Format/FormatTest.cpp:22599
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  Style.SpaceBeforeParensOptions.AfterRequiresClause = true;

curdeius wrote:
> This test seems redundant. Does it test something else than the above added 
> ones?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks updated this revision to Diff 385588.
HazardyKnusperkeks marked 2 inline comments as done.
HazardyKnusperkeks added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14300,6 +14300,36 @@
   verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
+
+  auto SpaceAfterRequires = getLLVMStyle();
+  SpaceAfterRequires.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat(
+  "template  void func(T) requires(trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}");
+  verifyFormat("template  void func(T) requires requires(T &) {\n"
+   "  typename T::size_type;\n"
+   "  { t.size() } -> std::same_as;\n"
+   "}\n"
+   "{}");
+  verifyFormat(
+  "template  void func(T) requires (trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
@@ -22565,6 +22595,21 @@
"}\n"
"{}",
Style);
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  Style.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  Style.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat("template \n"
+   "  requires (std::is_integral_v && std::is_signed_v)\n"
+   "void func(T) {}",
+   Style);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  Style);
 }
 
 TEST_F(FormatTest, StatementAttributeLikeMacros) {
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2477,6 +2477,13 @@
 void UnwrappedLineParser::parseConstraintExpression(
 unsigned int OriginalLevel) {
   // requires Id && Id || Id
+
+  if (FormatTok->Tok.is(tok::kw_requires)) {
+assert(FormatTok->Previous);
+assert(FormatTok->Previous->is(TT_RequiresClause));
+FormatTok->setType(TT_RequiresExpression);
+  }
+
   while (
   FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
 nextToken();
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3137,8 +3137,16 @@
   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.is(tok::kw_requires)) {
+  if (spaceRequiredBeforeParens(Right))
+return true;
+  if (Left.is(TT_RequiresClause))
+return Style.SpaceBeforeParensOptions.AfterRequiresClause;
+  else {
+assert(Left.is(TT_RequiresExpression));
+return Style.SpaceBeforeParensOptions.AfterRequiresExpression;
+  }
+}
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
   return true;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -855,6 +855,8 @@
 IO.mapOptional("AfterFunctionDeclarationName",
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
+IO.mapOptional("AfterRequiresClause", Spacing.AfterRequiresClause);
+IO.mapOptional("AfterRequiresExpression", Spacing.AfterRequiresExpression);
 IO.mapOptional("BeforeNonEmptyParentheses",

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3756
+  * ``bool AfterRequiresClause`` If ``true``, put space between requires 
keyword in a requires clause and
+opening parentheses, if is are one.
+

You meant "if there is one", right?



Comment at: clang/unittests/Format/FormatTest.cpp:14306
+  SpaceAfterRequires.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = true;

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.



Comment at: clang/unittests/Format/FormatTest.cpp:22599
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  Style.SpaceBeforeParensOptions.AfterRequiresClause = true;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113369/new/

https://reviews.llvm.org/D113369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

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

HazardyKnusperkeks created this revision.
HazardyKnusperkeks added reviewers: MyDeveloperDay, owenpan, crayroud.
HazardyKnusperkeks added a project: clang-format.
HazardyKnusperkeks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113369

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14300,6 +14300,36 @@
   verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
+
+  auto SpaceAfterRequires = getLLVMStyle();
+  SpaceAfterRequires.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  SpaceAfterRequires.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat(
+  "template  void func(T) requires(trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}");
+  verifyFormat("template  void func(T) requires requires(T &) {\n"
+   "  typename T::size_type;\n"
+   "  { t.size() } -> std::same_as;\n"
+   "}\n"
+   "{}");
+  verifyFormat(
+  "template  void func(T) requires (trait && trait) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  SpaceAfterRequires);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
@@ -22565,6 +22595,21 @@
"}\n"
"{}",
Style);
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  Style.SpaceBeforeParensOptions.AfterRequiresClause = true;
+  Style.SpaceBeforeParensOptions.AfterRequiresExpression = true;
+  verifyFormat("template \n"
+   "  requires (std::is_integral_v && std::is_signed_v)\n"
+   "void func(T) {}",
+   Style);
+  verifyFormat(
+  "template  void func(T) requires requires (T &) {\n"
+  "  typename T::size_type;\n"
+  "  { t.size() } -> std::same_as;\n"
+  "}\n"
+  "{}",
+  Style);
 }
 
 TEST_F(FormatTest, StatementAttributeLikeMacros) {
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2477,6 +2477,13 @@
 void UnwrappedLineParser::parseConstraintExpression(
 unsigned int OriginalLevel) {
   // requires Id && Id || Id
+
+  if (FormatTok->Tok.is(tok::kw_requires)) {
+assert(FormatTok->Previous);
+assert(FormatTok->Previous->is(TT_RequiresClause));
+FormatTok->setType(TT_RequiresExpression);
+  }
+
   while (
   FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
 nextToken();
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3137,8 +3137,16 @@
   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.is(tok::kw_requires)) {
+  if (spaceRequiredBeforeParens(Right))
+return true;
+  if (Left.is(TT_RequiresClause))
+return Style.SpaceBeforeParensOptions.AfterRequiresClause;
+  else {
+assert(Left.is(TT_RequiresExpression));
+return Style.SpaceBeforeParensOptions.AfterRequiresExpression;
+  }
+}
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
   return true;
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -855,6 +855,8 @@
 IO.mapOptional("AfterFunctionDeclarationName",
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
+IO.mapOptional("AfterRequiresClause", Spacing.AfterRequiresClause);
+