[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[Owen Pan 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 rG69209e30a716: [clang-format] 
AllowShortCompoundRequirementOnASingleLine (authored by Backl1ght, committed by 
owenpan).

Changed prior to commit:
  https://reviews.llvm.org/D139834?vs=482075=557878#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2767,6 +2767,40 @@
Style);
 }
 
+TEST_F(FormatTest, ShortCompoundRequirement) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_TRUE(Style.AllowShortCompoundRequirementOnASingleLine);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  { x + 1 } -> std::same_as;\n"
+   "};",
+   Style);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  { x + 1 } -> std::same_as;\n"
+   "  { x + 2 } -> std::same_as;\n"
+   "};",
+   Style);
+  Style.AllowShortCompoundRequirementOnASingleLine = false;
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  {\n"
+   "x + 1\n"
+   "  } -> std::same_as;\n"
+   "};",
+   Style);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  {\n"
+   "x + 1\n"
+   "  } -> std::same_as;\n"
+   "  {\n"
+   "x + 2\n"
+   "  } -> std::same_as;\n"
+   "};",
+   Style);
+}
+
 TEST_F(FormatTest, ShortCaseLabels) {
   FormatStyle Style = getLLVMStyle();
   Style.AllowShortCaseLabelsOnASingleLine = true;
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -154,6 +154,7 @@
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
+  CHECK_PARSE_BOOL(AllowShortCompoundRequirementOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -520,6 +520,8 @@
   // Try to merge records.
   if (TheLine->Last->is(TT_EnumLBrace)) {
 ShouldMerge = Style.AllowShortEnumsOnASingleLine;
+  } else if (TheLine->Last->is(TT_RequiresExpressionLBrace)) {
+ShouldMerge = Style.AllowShortCompoundRequirementOnASingleLine;
   } else if (TheLine->Last->isOneOf(TT_ClassLBrace, TT_StructLBrace)) {
 // NOTE: We use AfterClass (whereas AfterStruct exists) for both classes
 // and structs, but it seems that wrapping is still handled correctly
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -938,6 +938,8 @@
Style.AllowShortBlocksOnASingleLine);
 IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
Style.AllowShortCaseLabelsOnASingleLine);
+IO.mapOptional("AllowShortCompoundRequirementOnASingleLine",
+   Style.AllowShortCompoundRequirementOnASingleLine);
 IO.mapOptional("AllowShortEnumsOnASingleLine",
Style.AllowShortEnumsOnASingleLine);
 IO.mapOptional("AllowShortFunctionsOnASingleLine",
@@ -1441,6 +1443,7 @@
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -685,6 +685,25 @@
   /// \version 3.6
   

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[Emilia Kond via Phabricator via cfe-commits]

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

I see nothing wrong with this patch alone as it currently stands, since it's a 
quite simple change to the LineJoiner, and I see it as one of the final 
stopgaps in clang-format's support for requires clauses, and I think it 
definitely beats the current buggy behavior where behaviour is conflated with 
BraceWrapping.AfterFunction.
But I'd like other reviewers to maybe have another look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel added inline comments.



Comment at: clang/lib/Format/Format.cpp:809
Style.AllowShortCaseLabelsOnASingleLine);
+IO.mapOptional("AllowShortCompoundRequirementOnASingleLine",
+   Style.AllowShortCompoundRequirementOnASingleLine);

Backl1ght wrote:
> MyDeveloperDay wrote:
> > haven't we use "Requires" in other options? What is the definition of 
> > Compound?
> > 
> > I might be tempted for this to be AllShortRequiresOnASingleLine but then it 
> > be an enum with the following options
> > 
> > ```
> > Leave
> > Never
> > Always
> > Compound
> > ```
> There are some options related to requires like IndentRequiresClause, 
> RequiresClausePosition, etc. But as for single line and brace wrapping, 
> requires is not yet considered.
> 
> "Compound" is from Compound Requirements section in this page 
> https://en.cppreference.com/w/cpp/language/requires
> 
> I think this is a better way, I would try implementing it.
A //compound-requirement// is what the standard calls it, so I think the name 
is fine.

I'm not sure if collating it with other options as suggested is a good idea, 
since a //requires-expression// can contain multiple //compound-requirement//s, 
such as:

```
requires {
  { E1 } -> C;
  { E2 } -> D;
};
```

This //requires-expression// has two //compound-requirement//s, and two 
formatting of those two individually is what's being controlled by the option.

Although I will agree it should probably still be an enum, but in that case, it 
probably shouldn't start with "Allow"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:1265
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > Backl1ght wrote:
> > > MyDeveloperDay wrote:
> > > > why would the default be true, is that what happens today?
> > > yes
> > just to clarify so I'm sure (without me having to try it myself), if you 
> > hadn't introduce this option it would be the equivalent of true.
> > 
> > The only reason I say is we get complained at when we change the default 
> > from not doing something to doing something. Even if that means before we 
> > left it alone.
> > 
> > So mostly we normally find the options go through an evolution from   
> > bool->enum->struct, sometimes it can be better to introduce an enum so we 
> > can have "Leave" as the default
> > 
> > in such circumstances you let the old behaviour be the default, that way we 
> > know. That previously unformatted compound statements won't be touch in any 
> > way. 
> > 
> > ```
> > else if (Style.AllowShortCompoundRequirementOnASingleLine != Leave  && 
> > ...)
> > {
> > ```
> > 
> > Users then have to "positively" buy into your style change one way or 
> > another. Rather than us imposing a default even if that default seems 
> > perfectly reasonable.
> The `true` as default is correct.
Then if @HazardyKnusperkeks  you think this is ok, I'm ok with it too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:1265
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;

MyDeveloperDay wrote:
> Backl1ght wrote:
> > MyDeveloperDay wrote:
> > > why would the default be true, is that what happens today?
> > yes
> just to clarify so I'm sure (without me having to try it myself), if you 
> hadn't introduce this option it would be the equivalent of true.
> 
> The only reason I say is we get complained at when we change the default from 
> not doing something to doing something. Even if that means before we left it 
> alone.
> 
> So mostly we normally find the options go through an evolution from   
> bool->enum->struct, sometimes it can be better to introduce an enum so we can 
> have "Leave" as the default
> 
> in such circumstances you let the old behaviour be the default, that way we 
> know. That previously unformatted compound statements won't be touch in any 
> way. 
> 
> ```
> else if (Style.AllowShortCompoundRequirementOnASingleLine != Leave  && 
> ...)
> {
> ```
> 
> Users then have to "positively" buy into your style change one way or 
> another. Rather than us imposing a default even if that default seems 
> perfectly reasonable.
The `true` as default is correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:1265
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;

Backl1ght wrote:
> MyDeveloperDay wrote:
> > why would the default be true, is that what happens today?
> yes
just to clarify so I'm sure (without me having to try it myself), if you hadn't 
introduce this option it would be the equivalent of true.

The only reason I say is we get complained at when we change the default from 
not doing something to doing something. Even if that means before we left it 
alone.

So mostly we normally find the options go through an evolution from   
bool->enum->struct, sometimes it can be better to introduce an enum so we can 
have "Leave" as the default

in such circumstances you let the old behaviour be the default, that way we 
know. That previously unformatted compound statements won't be touch in any 
way. 

```
else if (Style.AllowShortCompoundRequirementOnASingleLine != Leave  && ...)
{
```

Users then have to "positively" buy into your style change one way or another. 
Rather than us imposing a default even if that default seems perfectly 
reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[Zhikai Zeng via Phabricator via cfe-commits]

Backl1ght added inline comments.



Comment at: clang/lib/Format/Format.cpp:809
Style.AllowShortCaseLabelsOnASingleLine);
+IO.mapOptional("AllowShortCompoundRequirementOnASingleLine",
+   Style.AllowShortCompoundRequirementOnASingleLine);

MyDeveloperDay wrote:
> haven't we use "Requires" in other options? What is the definition of 
> Compound?
> 
> I might be tempted for this to be AllShortRequiresOnASingleLine but then it 
> be an enum with the following options
> 
> ```
> Leave
> Never
> Always
> Compound
> ```
There are some options related to requires like IndentRequiresClause, 
RequiresClausePosition, etc. But as for single line and brace wrapping, 
requires is not yet considered.

"Compound" is from Compound Requirements section in this page 
https://en.cppreference.com/w/cpp/language/requires

I think this is a better way, I would try implementing it.



Comment at: clang/lib/Format/Format.cpp:1265
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;

MyDeveloperDay wrote:
> why would the default be true, is that what happens today?
yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:809
Style.AllowShortCaseLabelsOnASingleLine);
+IO.mapOptional("AllowShortCompoundRequirementOnASingleLine",
+   Style.AllowShortCompoundRequirementOnASingleLine);

haven't we use "Requires" in other options? What is the definition of Compound?

I might be tempted for this to be AllShortRequiresOnASingleLine but then it be 
an enum with the following options

```
Leave
Never
Always
Compound
```



Comment at: clang/lib/Format/Format.cpp:1265
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;

why would the default be true, is that what happens today?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

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

HazardyKnusperkeks added a comment.

See the comments on D139786 . And you can 
base your commits on each other, and then add a child rsp. parent revision here 
in phabricator, that will prevent merge conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139834

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

[PATCH] D139834: [clang-format] AllowShortCompoundRequirementOnASingleLine

[Zhikai Zeng via Phabricator via cfe-commits]

Backl1ght created this revision.
Backl1ght added reviewers: HazardyKnusperkeks, rymiel.
Backl1ght added a project: clang-format.
Herald added a project: All.
Backl1ght requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

clang-format brace wrapping did not take requires into consideration, compound 
requirements will be affected by BraceWrapping.AfterFunction.

I plan to add 3 options AllowShortRequiresExpressionOnASingleLine, 
AllowShortCompoundRequirementOnASingleLine and 
BraceWrapping.AfterRequiresExpression.

This patch is for AllowShortCompoundRequirementOnASingleLine.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139834

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2899,6 +2899,40 @@
Style);
 }
 
+TEST_F(FormatTest, ShortCompoundRequirement) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_TRUE(Style.AllowShortCompoundRequirementOnASingleLine);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  { x + 1 } -> std::same_as;\n"
+   "};",
+   Style);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  { x + 1 } -> std::same_as;\n"
+   "  { x + 2 } -> std::same_as;\n"
+   "};",
+   Style);
+  Style.AllowShortCompoundRequirementOnASingleLine = false;
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  {\n"
+   "x + 1\n"
+   "  } -> std::same_as;\n"
+   "};",
+   Style);
+  verifyFormat("template \n"
+   "concept c = requires(T x) {\n"
+   "  {\n"
+   "x + 1\n"
+   "  } -> std::same_as;\n"
+   "  {\n"
+   "x + 2\n"
+   "  } -> std::same_as;\n"
+   "};",
+   Style);
+}
+
 TEST_F(FormatTest, ShortCaseLabels) {
   FormatStyle Style = getLLVMStyle();
   Style.AllowShortCaseLabelsOnASingleLine = true;
Index: clang/unittests/Format/ConfigParseTest.cpp
===
--- clang/unittests/Format/ConfigParseTest.cpp
+++ clang/unittests/Format/ConfigParseTest.cpp
@@ -147,6 +147,7 @@
   CHECK_PARSE_BOOL(AllowAllArgumentsOnNextLine);
   CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine);
   CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine);
+  CHECK_PARSE_BOOL(AllowShortCompoundRequirementOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(BinPackArguments);
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -502,6 +502,8 @@
   // Try to merge records.
   if (TheLine->Last->is(TT_EnumLBrace)) {
 ShouldMerge = Style.AllowShortEnumsOnASingleLine;
+  } else if (TheLine->Last->is(TT_CompoundRequirementLBrace)) {
+ShouldMerge = Style.AllowShortCompoundRequirementOnASingleLine;
   } else if (TheLine->Last->isOneOf(TT_ClassLBrace, TT_StructLBrace)) {
 // NOTE: We use AfterClass (whereas AfterStruct exists) for both classes
 // and structs, but it seems that wrapping is still handled correctly
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -806,6 +806,8 @@
Style.AllowShortBlocksOnASingleLine);
 IO.mapOptional("AllowShortCaseLabelsOnASingleLine",
Style.AllowShortCaseLabelsOnASingleLine);
+IO.mapOptional("AllowShortCompoundRequirementOnASingleLine",
+   Style.AllowShortCompoundRequirementOnASingleLine);
 IO.mapOptional("AllowShortEnumsOnASingleLine",
Style.AllowShortEnumsOnASingleLine);
 IO.mapOptional("AllowShortFunctionsOnASingleLine",
@@ -1260,6 +1262,7 @@
   LLVMStyle.AllowAllParametersOfDeclarationOnNextLine = true;
   LLVMStyle.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
+  LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
   LLVMStyle.AllowShortEnumsOnASingleLine = true;
   LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
Index: