[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

Hi,

consider the following code

  myFunc([](int x) {
if (x)
call();
  });

When activating your BeforeLambdaBody option, it changes the code to this 
(breaking the start of the lambda to a new line)

  myFunc(
[](int x)
{
if (x)
call();
});

Another issue if I declare the lambda as noexcept, clang-format changes the 
code to this (looks like it's not detecting and applying your option):

  myFunc([](int x) noexcept {
if (x)
call();
  });


Repository:
  rC Clang

https://reviews.llvm.org/D44609



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

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

Actually, without your change to HasMultipleNestedBlocks, I'm almost getting 
the expected result: Lambda body is correctly indented (and not by function 
name's length).
The only thing not working (and it's not either way, with or without your 
change to HasMultipleNestedBlocks), is nested lambdas. The body is not properly 
indented (but it's not that bad).

About the "noexcept" issue, it might not be related to your patch, I'm not 
sure. Although the "mutable" tag is working as expected.


Repository:
  rC Clang

https://reviews.llvm.org/D44609



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

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

Ok I found the issue with noexcept, it's not related to your patch.
There is a missing

  case tok::kw_noexcept:

in

  UnwrappedLineParser::tryToParseLambda


Repository:
  rC Clang

https://reviews.llvm.org/D44609



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

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

I fixed it like this (not sure it's 100% correct though!!)

State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 
1;
if (Style.BraceWrapping.BeforeLambdaBody && Current.Next != nullptr &&
Current.Tok.getKind() == tok::TokenKind::l_paren &&
Current.BlockParameterCount >= 1) {
// Search for any parameter that is a lambda
  auto const *nextTok = Current.Next;
  while (nextTok != nullptr) {
if (nextTok->is(TT_LambdaLSquare)) {
  State.Stack.back().HasMultipleNestedBlocks = true;
  break;
}
nextTok = nextTok->Next;
  }
}

It works for all cases I'm using to test:

  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 still think it's a hack (changing "HasMultipleNestedBlocks"), but it seems to 
work.


Repository:
  rC Clang

https://reviews.llvm.org/D44609



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://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

[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

[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

[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

[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] D44609: [clang-format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Christophe Calmejane via Phabricator via cfe-commits]

christophe-calmejane added a comment.

In D44609#1875172 , @Wawha wrote:

> In D44609#1871612 , @MyDeveloperDay 
> wrote:
>
> > Correct follow that description on how to request commit access
>
>
> It's done. I have the commit right, and push that change on github : 
> https://github.com/llvm/llvm-project/commit/fa0118e6e588fe303b08e7e06ba28ac1f8d50c68
>
> Thank you for the review and advices!


Nice to finally see this patch integrated!

But, it looks like you didn't include all the test case I posted 1.5y ago in 
this post, that are still problematic and not formatting correctly with your 
patch:
For example, this simple case do not format correctly:

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

The output is:

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

although I would expect to see

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

See my proposed fix in the discussion, but note that I don't think it's clean 
enough to be accepted :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44609



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