[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG37c2233097ac: [clang-format] [PR50702] Lamdba processing 
does not respect AfterClass and… (authored by MyDeveloperDay).

Changed prior to commit:
  https://reviews.llvm.org/D104222?vs=352193=354671#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104222

Files:
  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
@@ -19596,8 +19596,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19802,6 +19801,35 @@
"});",
LLVMWithBeforeLambdaBody);
 
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+
+  verifyFormat("auto select = [this]() -> const Library::Object *\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("auto select = [this]() -> const Library::Object &\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("auto select = [this]() -> std::unique_ptr\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
+
   // Lambdas with different indentation styles.
   Style = getLLVMStyleWithColumns(100);
   EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3579,42 +3579,11 @@
   return Tok.Children.empty() && ShortLambdaOption != FormatStyle::SLS_None;
 }
 
-static bool
-isItAInlineLambdaAllowed(const FormatToken ,
- FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  return (ShortLambdaOption == FormatStyle::SLS_Inline &&
-  IsFunctionArgument(Tok)) ||
- (ShortLambdaOption == FormatStyle::SLS_All);
-}
-
-static bool isOneChildWithoutMustBreakBefore(const FormatToken ) {
-  if (Tok.Children.size() != 1)
-return false;
-  FormatToken *curElt = Tok.Children[0]->First;
-  while (curElt) {
-if (curElt->MustBreakBefore)
-  return false;
-curElt = curElt->Next;
-  }
-  return true;
-}
 static bool isAllmanLambdaBrace(const FormatToken ) {
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
   !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
 }
 
-static bool isAllmanBraceIncludedBreakableLambda(
-const FormatToken , FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  if (!isAllmanLambdaBrace(Tok))
-return false;
-
-  if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption))
-return false;
-
-  return !isItAInlineLambdaAllowed(Tok, ShortLambdaOption) ||
- !isOneChildWithoutMustBreakBefore(Tok);
-}
-
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine ,
  const FormatToken ) {
   const FormatToken  = *Right.Previous;
@@ -3776,13 +3745,6 @@
   if (Right.is(TT_InlineASMBrace))
 return Right.HasUnescapedNewline;
 
-  auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody &&
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;
-  }
-
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
 return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
(Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
@@ -3805,6 +3767,11 @@
   return true;
   }
 
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
+  Left.isOneOf(tok::star, tok::amp, tok::ampamp, TT_TemplateCloser)) {
+return true;
+  }
+
   // Put multiple Java annotation on a new line.
   if ((Style.Language == FormatStyle::LK_Java ||
Style.Language == FormatStyle::LK_JavaScript) &&
@@ -4209,7 +4176,7 @@
 return false;
 
   auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
- 

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

> If you did The only difference (with this current patch) would be the 
> positioning of the first arguments on `paramBeforeAndAfterLambda` and 
> `doubleLambda` (I'm not sure the reason for why that is)

@MyDeveloperDay The reason is the fix I talked about in the other thread 
(https://reviews.llvm.org/D44609) that I proposed for LLVM 7.0.0. But last time 
I checked, my patch couldn't be applied anymore due to massive changes in the 
code. I don't have enough knowledge of clang-format to clearly follow where 
went the code my patch applies to, I originally fixed the issue blindly by 
stepping into the code :)
Nevertheless, I think the issue is still present (since my patch was not 
applied to the base code) and it might still be related to this reason. See my 
comment that fixed the issue back then 
(https://reviews.llvm.org/D44609#1255395).

It would be really awesome if this could finally be fixed so we have a 
consistent formatting for lambdas!


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

In D104222#2821375 , @MyDeveloperDay 
wrote:

>> @MyDeveloperDay 
>> Of course, sorry for the delay, here it is: 
>> https://github.com/christophe-calmejane/Hive/blob/master/.clang-format
>
> You don't have `AllowShortLambdasOnASingleLine` set to be `false` or `None`

Just a clarification, this file is meant to be used with clang-format v7.0.0 
along with my small patch fixing formatting issues that were not included in 
the original merge (see the other post). This means it's using the defaults 
from back then ;)
I never had a chance to upgrade clang-format because lambda formatting was 
broken (some of my test cases were failing). I will try to build your latest 
patch and see how it goes. I would expect my project re-formatting to not 
change much :D
Thanks for your work btw.


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> @MyDeveloperDay 
> Of course, sorry for the delay, here it is: 
> https://github.com/christophe-calmejane/Hive/blob/master/.clang-format

You don't have `AllowShortLambdasOnASingleLine` set to be `false` or `None`

If you did The only difference (with this current patch) would be the 
positioning of the first arguments on `paramBeforeAndAfterLambda` and 
`doubleLambda` (I'm not sure the reason for why that is)

F17423148: image.png 


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

In D104222#2819403 , @MyDeveloperDay 
wrote:

> @christophe-calmejane are you able to post your .clang-format styles that you 
> are using, I'm struggling to see where its not matching your style (other 
> than brace styles on the function and argument placing)

Of course, sorry for the delay, here it is: 
https://github.com/christophe-calmejane/Hive/blob/master/.clang-format


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D104222#2820001 , 
@HazardyKnusperkeks wrote:

> Just asking: Would it be different if there is `auto`, `decltype(auto)`, 
> `decltype(a+b)`, `typename` or `template` as trailing return type?

It would be good if you could give a small example, just so I can clarify what 
you mean,

but I think these could easily be corner cases which at least for a current non 
parsed based solution would could potentially fill with mustBreak rules..


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

For this case:

>   return iter::chain.from_iterable(
> [](auto&& ite) -> auto&
> {
>   return ite.second;
> });

The code has not been classified as a Lambda

  AnnotatedTokens(L=0):
   M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=return L=6 PPK=2 FakeLParens= 
FakeRParens=0 II=0x24bf7e8 Text='return'
   M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=identifier L=11 PPK=2 
FakeLParens=0/ FakeRParens=0 II=0x24c35c0 Text='iter'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=coloncolon L=13 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='::'
   M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=520 Name=identifier L=18 PPK=2 
FakeLParens= FakeRParens=0 II=0x24c3738 Text='chain'
   M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=170 Name=period L=19 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='.'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=identifier L=32 PPK=2 
FakeLParens= FakeRParens=0 II=0x24c3760 Text='from_iterable'
   M=0 C=0 T=Unknown S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=33 PPK=1 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
   M=0 C=1 T=ArrayInitializerLSquare S=0 F=0 B=0 BK=0 P=59 Name=l_square L=34 
PPK=2 FakeLParens=1/ FakeRParens=0 II=0x0 Text='['
   M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=r_square L=35 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text=']'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=36 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
   M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=auto L=40 PPK=2 FakeLParens= 
FakeRParens=0 II=0x24bf510 Text='auto'
   M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=250 Name=ampamp L=43 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='&&'
   M=0 C=1 T=StartOfName S=0 F=0 B=0 BK=0 P=260 Name=identifier L=46 PPK=2 
FakeLParens= FakeRParens=0 II=0x24c3780 Text='ite'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=63 Name=r_paren L=47 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')'
   M=0 C=1 T=LambdaArrow S=1 F=0 B=0 BK=0 P=150 Name=arrow L=50 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='->'
   M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=43 Name=auto L=55 PPK=2 FakeLParens= 
FakeRParens=0 II=0x24bf510 Text='auto'
   M=0 C=1 T=PointerOrReference S=1 F=0 B=0 BK=0 P=230 Name=amp L=57 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='&'
   M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=48 Name=l_brace L=59 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='{'
   M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=80 PPK=2 FakeLParens= 
FakeRParens=1 II=0x0 Text='}'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=81 PPK=2 FakeLParens= 
FakeRParens=1 II=0x0 Text=')'
   M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=82 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=';'

It doesn't see this

  M=0 C=0 T=Unknown S=1 F=0 B=0 BK=1 P=48 Name=l_brace L=59 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='{'

As a LamdaLBrace

Again I would say this was actually a failure elsewhere


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> And also another different in that case (without lambda):
>
>   ASSERT_NO_THROW(
>   {
>   iterator += 507408;
>   });
>
> is now format like this:
>
>   ASSERT_NO_THROW({ iterator += 507408; });

I think this shows where your change made an assumption that this was a lambda, 
but its not necessarily is as there is no TT_LamdbaLBrace, I would argue your 
change was the regression even if looks nicer (in your view)

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=15 PPK=2 FakeLParens= 
FakeRParens=0 II=0xad8a50 Text='ASSERT_NO_THROW'
 M=0 C=0 T=Unknown S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=16 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=1 P=140 Name=l_brace L=17 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='{'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=39 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='}'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=40 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=41 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=';'


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Part of the problem here is we are not really parsing the Lambda but trying to 
break based on token annotation only. This can tend to be a bit fragile 
(normally around corner cases)

The previous approach I think was not constrained enough to just lambdas which 
is why we had the bleed into other things, also when we added this we didn't 
really test that it didn't have any impact on anything else.

Actually this has been a problem when adding new options. I keep meaning to 
extend the `LLVMDefaultStyle`  `GNUDefaultStyle`  `MozillaDefaultStyle` tests 
to cover all behaviour (if/else/while/do/for/etc..) but also to find a way that 
when a new option is added we can validate that non of the other aspects of a 
style aren't broken when using that. (not an easy problem to fix)

I'm keen for this NOT to break the AfterClass, AfterNamespace issue, (which is 
a regression)... we can continue to chip away at corner cases or revert the 
original change completely. (which I also don't want to do)

likely trying to add add parseXXX() functions in UnwrappedLineParser would 
likely take much longer. I feel like that might offer the better solution but 
think I would prefer we used that to replace this and gravitate slowly towards 
a more correct case


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added a comment.

Indeed, I found another case with a regression:

  return iter::chain.from_iterable(
[](auto&& ite) -> auto&
{
  return ite.second;
});

it's format all inline.

And also another different in that case (without lambda):

ASSERT_NO_THROW(
{
iterator += 507408;
});

is now format like this:

ASSERT_NO_THROW({ iterator += 507408; });


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

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

HazardyKnusperkeks added a comment.

Just asking: Would it be different if there is `auto`, `decltype(auto)`, 
`decltype(a+b)`, `typename` or `template` as trailing return type?


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 352193.
MyDeveloperDay added a comment.

Add support for TemplateCloser before LambdaBrace


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

https://reviews.llvm.org/D104222

Files:
  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
@@ -18856,8 +18856,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19061,6 +19060,35 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
+
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+
+  verifyFormat("auto select = [this]() -> const Library::Object *\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("auto select = [this]() -> const Library::Object &\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("auto select = [this]() -> std::unique_ptr\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3557,42 +3557,11 @@
   return Tok.Children.empty() && ShortLambdaOption != FormatStyle::SLS_None;
 }
 
-static bool
-isItAInlineLambdaAllowed(const FormatToken ,
- FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  return (ShortLambdaOption == FormatStyle::SLS_Inline &&
-  IsFunctionArgument(Tok)) ||
- (ShortLambdaOption == FormatStyle::SLS_All);
-}
-
-static bool isOneChildWithoutMustBreakBefore(const FormatToken ) {
-  if (Tok.Children.size() != 1)
-return false;
-  FormatToken *curElt = Tok.Children[0]->First;
-  while (curElt) {
-if (curElt->MustBreakBefore)
-  return false;
-curElt = curElt->Next;
-  }
-  return true;
-}
 static bool isAllmanLambdaBrace(const FormatToken ) {
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
   !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
 }
 
-static bool isAllmanBraceIncludedBreakableLambda(
-const FormatToken , FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  if (!isAllmanLambdaBrace(Tok))
-return false;
-
-  if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption))
-return false;
-
-  return !isItAInlineLambdaAllowed(Tok, ShortLambdaOption) ||
- !isOneChildWithoutMustBreakBefore(Tok);
-}
-
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine ,
  const FormatToken ) {
   const FormatToken  = *Right.Previous;
@@ -3754,13 +3723,6 @@
   if (Right.is(TT_InlineASMBrace))
 return Right.HasUnescapedNewline;
 
-  auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody &&
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;
-  }
-
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
 return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
(Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
@@ -3783,6 +3745,11 @@
   return true;
   }
 
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
+  Left.isOneOf(tok::star, tok::amp, tok::ampamp, TT_TemplateCloser)) {
+return true;
+  }
+
   // Put multiple Java annotation on a new line.
   if ((Style.Language == FormatStyle::LK_Java ||
Style.Language == FormatStyle::LK_JavaScript) &&
@@ -4186,7 +4153,7 @@
 return false;
 
   auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody) {
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) {
 if (isAllmanLambdaBrace(Left))
   return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption);
 if (isAllmanLambdaBrace(Right))
@@ 

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added a comment.

In D104222#2819422 , @MyDeveloperDay 
wrote:

> Add some of the failing cases identified by @Wawha

Thank for the fix. I test and discover a new case with a regression:

auto createObj = [this] -> std::unique_ptr
{
  return std::make_unique();
};

Like & and * a template return ending with > is not handle.


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added a comment.

In D104222#2819358 , @MyDeveloperDay 
wrote:

> @Wawha your cases could be covered by the following (in mustBreakBefore)
>
>   if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
>   Left.isOneOf(tok::star, tok::amp, tok::ampamp)) {
> return true;
>   }
>
> As I think  its the presence of & and * that causes it to not wrap.
>
> By all means if you would prefer to commandeer the patch I'm ok with that. I 
> was just on a bug fixing day. If you'd like to take this over and add the 
> unit tests @christophe-calmejane suggest, I'm happy to be the reviewer

Indeed, & and * could be the root cause.
About fixing that, I won't have the time in the next days. It will take time 
for me to re-read and re-understand that code.
But I could find time to test patch or modification in my code to check that 
there is no regression.


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 352136.
MyDeveloperDay added a comment.

Add some of the failing cases identified by @Wawha


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

https://reviews.llvm.org/D104222

Files:
  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
@@ -18856,8 +18856,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19061,6 +19060,29 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
+
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+
+  verifyFormat("auto select = [this]() -> const Library::Object *\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("auto select = [this]() -> const Library::Object &\n"
+   "{\n"
+   "  return MyAssignment::SelectFromList(this);\n"
+   "};\n",
+   LLVMWithBeforeLambdaBody);
+
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3557,42 +3557,11 @@
   return Tok.Children.empty() && ShortLambdaOption != FormatStyle::SLS_None;
 }
 
-static bool
-isItAInlineLambdaAllowed(const FormatToken ,
- FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  return (ShortLambdaOption == FormatStyle::SLS_Inline &&
-  IsFunctionArgument(Tok)) ||
- (ShortLambdaOption == FormatStyle::SLS_All);
-}
-
-static bool isOneChildWithoutMustBreakBefore(const FormatToken ) {
-  if (Tok.Children.size() != 1)
-return false;
-  FormatToken *curElt = Tok.Children[0]->First;
-  while (curElt) {
-if (curElt->MustBreakBefore)
-  return false;
-curElt = curElt->Next;
-  }
-  return true;
-}
 static bool isAllmanLambdaBrace(const FormatToken ) {
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
   !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
 }
 
-static bool isAllmanBraceIncludedBreakableLambda(
-const FormatToken , FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  if (!isAllmanLambdaBrace(Tok))
-return false;
-
-  if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption))
-return false;
-
-  return !isItAInlineLambdaAllowed(Tok, ShortLambdaOption) ||
- !isOneChildWithoutMustBreakBefore(Tok);
-}
-
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine ,
  const FormatToken ) {
   const FormatToken  = *Right.Previous;
@@ -3754,13 +3723,6 @@
   if (Right.is(TT_InlineASMBrace))
 return Right.HasUnescapedNewline;
 
-  auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody &&
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;
-  }
-
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
 return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
(Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
@@ -3783,6 +3745,11 @@
   return true;
   }
 
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
+  Left.isOneOf(tok::star, tok::amp, tok::ampamp)) {
+return true;
+  }
+
   // Put multiple Java annotation on a new line.
   if ((Style.Language == FormatStyle::LK_Java ||
Style.Language == FormatStyle::LK_JavaScript) &&
@@ -4186,7 +4153,7 @@
 return false;
 
   auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody) {
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) {
 if (isAllmanLambdaBrace(Left))
   return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption);
 if (isAllmanLambdaBrace(Right))
@@ -4198,7 +4165,6 @@
  Right.isMemberAccess() ||
  Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless,
tok::colon, tok::l_square, tok::at) ||
- 

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@christophe-calmejane are you able to post your .clang-format styles that you 
are using, I'm struggling to see where its not matching your style (other than 
brace styles on the function and argument placing)


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@Wawha your cases could be covered by the following (in mustBreakBefore)

  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
  Left.isOneOf(tok::star, tok::amp, tok::ampamp)) {
return true;
  }

As I think  its the presence of & and * that causes it to not wrap.

By all means if you would prefer to commandeer the patch I'm ok with that. I 
was just on a bug fixing day. If you'd like to take this over and add the unit 
tests @christophe-calmejane suggest, I'm happy to be the reviewer


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

In D104222#2819324 , @Wawha wrote:

> Hi @christophe-calmejane.
> I test your code with a old version (few time after the merge of 
> https://reviews.llvm.org/D44609), and the latest version (commit: 
> e0c382a9d5a0 
> ), and 
> in both cases I have the same result, which is near your output:
>
> I though I add multiples cases, but looking at UnitTests with inline lambda 
> "None", I see only few tests, perhaps some are missing.

I posted a fix for the incorrect formatting you have in your output (in your 
original review post), but like I said back then, I thought the fix was not 
clean enough to be included. Nevertheless it did fix all cases and formatting 
was perfect for my whole test suite.
I using this "patched" version since then as it's the only one with the correct 
lambda formatting. I tried to reapply my patch on a recent version.. of course 
it failed as there were too many changes in clang-format. I don't have enough 
knowledge on this project to be able to clearly understand how it works and fix 
the formatting on the latest version (but I wish I could)... at least not 
without having to spend too much time on it :(
For the record, my patch over clang-format 7.0 can be found here: 
https://www.kikisoft.com/Hive/clang-format/


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added a comment.

Hi @christophe-calmejane.
I test your code with a old version (few time after the merge of 
https://reviews.llvm.org/D44609), and the latest version (commit: e0c382a9d5a0 
), and in 
both cases I have the same result, which is near your output:

  noOtherParams(
[](int x)
{
call();
});
  paramBeforeLambda(
8,
[](int x)
{
call();
});
  paramAfterLambda(
[](int x)
{
call();
},
5);
  paramBeforeAndAfterLambda(
8,
[](int x)
{
call();
},
5);
  doubleLambda(
8,
[](int x)
{
call();
},
6,
[](float f)
{
return f * 2;
},
5);
  nestedLambda1(
[](int x)
{
return [](float f)
{
return f * 2;
};
});
  nestedLambda2(
[](int x)
{
call(
[](float f)
{
return f * 2;
});
});
  noExceptCall(
[](int x) noexcept
{
call();
});
  mutableCall(
[](int x) mutable
{
call();
});
  
  funcCallFunc(
call(),
[](int x)
{
call();
});
  
  auto const l = [](char v)
  {
if(v)
call();
  };
  void f()
  {
return [](char v)
{
if(v)
return v++;
};
  }

The only different is for cases with multiples parameters.

  paramBeforeLambda(
8,
[](int x)
{
call();
}

I though I add multiples cases, but looking at UnitTests with inline lambda 
"None", I see only few tests, perhaps some are missing.


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

Hi,

Due to the pandemic, I wasn't able to look at https://reviews.llvm.org/D44609 
which got broken since it was pushed to LLVM. Nevertheless I commented in that 
post with a rather complete unit tests that should pass if any changes are to 
be pushed to that part of the code.

Here is the test suite:

  noOtherParams([](int x){call();});
  paramBeforeLambda(8,[](int x){call();});
  paramAfterLambda([](int x){call();},5);
  paramBeforeAndAfterLambda(8,[](int x){call();},5);
  doubleLambda(8,[](int x){call();},6,[](float f){return f*2;},5);
  nestedLambda1([](int x){return [](float f){return f*2;};});
  nestedLambda2([](int x){ call([](float f){return f*2;});});
  noExceptCall([](int x) noexcept {call();});
  mutableCall([](int x) mutable{call();});
  
  funcCallFunc(call(),[](int x){call();});
  
  auto const l=[](char v){if(v)call();};
  void f() { return [](char v){ if(v) return v++;}; }

And here is how it should be formatted:

  noOtherParams(
  [](int x)
  {
  call();
  });
  paramBeforeLambda(8,
  [](int x)
  {
  call();
  });
  paramAfterLambda(
  [](int x)
  {
  call();
  },
  5);
  paramBeforeAndAfterLambda(8,
  [](int x)
  {
  call();
  },
  5);
  doubleLambda(8,
  [](int x)
  {
  call();
  },
  6,
  [](float f)
  {
  return f * 2;
  },
  5);
  nestedLambda1(
  [](int x)
  {
  return [](float f)
  {
  return f * 2;
  };
  });
  nestedLambda2(
  [](int x)
  {
  call(
  [](float f)
  {
  return f * 2;
  });
  });
  noExceptCall(
  [](int x) noexcept
  {
  call();
  });
  mutableCall(
  [](int x) mutable
  {
  call();
  });
  
  funcCallFunc(call(),
  [](int x)
  {
  call();
  });
  
  auto const l = [](char v)
  {
  if (v)
  call();
  };
  void f()
  {
  return [](char v)
  {
  if (v)
  return v++;
  };
  }

I couldn't find the time to test this case with the latest clang-format 
release, but I guess it's still broken and doesn't match the expected output.
Anyway, this test suite is helpful as it covers most cases, and if someone with 
enough knowledge of LLVM is willing to add this to the unit tests it would help 
a lot.

Best regards,
Chris


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added a comment.

In D104222#2817147 , @MyDeveloperDay 
wrote:

> @Wawha do you have a project that perhaps uses this work your did? I would 
> like to ensure I didn't break anything

@MyDeveloperDay Good idea!
I just test on my code, and I see 2 types of regressions due to that patch.
This code:

auto select = [this]() -> const Library::Object*
{
return MyAssignment::SelectFromList(this);
};

Become:

auto select = [this]() -> const Library::Object* {
return MyAssignment::SelectFromList(this);
};

And this code,

auto Items()
{
return iter::imap(
[this](const std::unique_ptr& iItem) -> T&
{
return *GetItem(*iItem)->Item;
},
m_Root->Items());
}

Become:

auto Items()
{
return iter::imap(
[this](const std::unique_ptr& iItem) -> T& {
return *GetItem(*iItem)->Item;
},
m_Root->Items());
}

I'm surprise, because the both cases, should be present inside the UnitTest.


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added a comment.

LGTM


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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 351895.
MyDeveloperDay added a comment.

Remove unused functions


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

https://reviews.llvm.org/D104222

Files:
  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
@@ -18856,8 +18856,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19061,6 +19060,16 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
+
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3557,42 +3557,11 @@
   return Tok.Children.empty() && ShortLambdaOption != FormatStyle::SLS_None;
 }
 
-static bool
-isItAInlineLambdaAllowed(const FormatToken ,
- FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  return (ShortLambdaOption == FormatStyle::SLS_Inline &&
-  IsFunctionArgument(Tok)) ||
- (ShortLambdaOption == FormatStyle::SLS_All);
-}
-
-static bool isOneChildWithoutMustBreakBefore(const FormatToken ) {
-  if (Tok.Children.size() != 1)
-return false;
-  FormatToken *curElt = Tok.Children[0]->First;
-  while (curElt) {
-if (curElt->MustBreakBefore)
-  return false;
-curElt = curElt->Next;
-  }
-  return true;
-}
 static bool isAllmanLambdaBrace(const FormatToken ) {
   return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
   !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
 }
 
-static bool isAllmanBraceIncludedBreakableLambda(
-const FormatToken , FormatStyle::ShortLambdaStyle ShortLambdaOption) {
-  if (!isAllmanLambdaBrace(Tok))
-return false;
-
-  if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption))
-return false;
-
-  return !isItAInlineLambdaAllowed(Tok, ShortLambdaOption) ||
- !isOneChildWithoutMustBreakBefore(Tok);
-}
-
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine ,
  const FormatToken ) {
   const FormatToken  = *Right.Previous;
@@ -3754,13 +3723,6 @@
   if (Right.is(TT_InlineASMBrace))
 return Right.HasUnescapedNewline;
 
-  auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody &&
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;
-  }
-
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
 return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
(Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
@@ -4186,7 +4148,7 @@
 return false;
 
   auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody) {
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) {
 if (isAllmanLambdaBrace(Left))
   return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption);
 if (isAllmanLambdaBrace(Right))
@@ -4198,7 +4160,6 @@
  Right.isMemberAccess() ||
  Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless,
tok::colon, tok::l_square, tok::at) ||
- (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) ||
  (Left.is(tok::r_paren) &&
   Right.isOneOf(tok::identifier, tok::kw_const)) ||
  (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@Wawha do you have a project that perhaps uses this work your did? I would like 
to ensure I didn't break anything


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[Francois JEAN via Phabricator via cfe-commits]

Wawha added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3760
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;

Without this code, you should be able to remove functions 
isAllmanBraceIncludedBreakableLambda(), isItAInlineLambdaAllowed() and 
isOneChildWithoutMustBreakBefore().

Thank you for the fix.

When I implement that part, I have some difficulty to handle inline lambda and 
BreakBeforeLambdaBody. At the beginning, I implement the solution without 
inline lambda option (not yet present). But I have to handle it before merging 
the patch. And, this modification was added at this moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104222

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

[PATCH] D104222: [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: curdeius, HazardyKnusperkeks, Wawha, krasimir.
MyDeveloperDay added projects: clang, clang-format.
MyDeveloperDay requested review of this revision.

https://bugs.llvm.org/show_bug.cgi?id=50702

I believe D44609: [clang-format] New option BeforeLambdaBody to manage lambda 
line break inside function parameter call (in Allman style) 
 may be too aggressive with brace wrapping 
rules which doesn't always apply to Lamdbas

The introduction of BeforeLambdaBody and AllowShortLambdasOnASingleLine has 
impact on brace handling on other block types, which I suspect we didn't see 
before as people may not be using the BeforeLambdaBody  style

>From what I can tell this can be seen by the unit test I change as its not 
>honouring the orginal LLVM brace wrapping style for the `Fct()` function

I added a unit test from PR50702 and have removed some of the code (which has 
zero impact on the unit test, which kind of suggests its unnecessary), some 
additional attempt has been made to try and ensure we'll only break on what is 
actually a LamdbaLBrace


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104222

Files:
  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
@@ -18856,8 +18856,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19061,6 +19060,16 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
+
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
 }
 
 TEST_F(FormatTest, LambdaWithLineComments) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3754,13 +3754,6 @@
   if (Right.is(TT_InlineASMBrace))
 return Right.HasUnescapedNewline;
 
-  auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody &&
-  (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) ||
-   isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) {
-return true;
-  }
-
   if (isAllmanBrace(Left) || isAllmanBrace(Right))
 return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) ||
(Line.startsWith(tok::kw_typedef, tok::kw_enum) &&
@@ -4186,7 +4179,7 @@
 return false;
 
   auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine;
-  if (Style.BraceWrapping.BeforeLambdaBody) {
+  if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) {
 if (isAllmanLambdaBrace(Left))
   return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption);
 if (isAllmanLambdaBrace(Right))
@@ -4198,7 +4191,6 @@
  Right.isMemberAccess() ||
  Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless,
tok::colon, tok::l_square, tok::at) ||
- (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) ||
  (Left.is(tok::r_paren) &&
   Right.isOneOf(tok::identifier, tok::kw_const)) ||
  (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) ||


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -18856,8 +18856,7 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
-  verifyFormat("void Fct()\n"
-   "{\n"
+  verifyFormat("void Fct() {\n"
"  return {[]()\n"
"  {\n"
"return 17;\n"
@@ -19061,6 +19060,16 @@
"  });\n"
"});",
LLVMWithBeforeLambdaBody);
+
+  LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine =
+  FormatStyle::ShortLambdaStyle::SLS_None;
+  verifyFormat("namespace test {\n"
+   "class Test {\n"
+   "public:\n"
+   "  Test() = default;\n"
+   "};\n"
+   "} // namespace test",
+   LLVMWithBeforeLambdaBody);
 }
 
 TEST_F(FormatTest,