[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

@djasper @klimek could you chime in please? This patch strives to cleanup a 
quirk in lambda formatting which pushes code too far to the right. Here is the 
problematic case:

  void f() {
something.something.something.Method(some_arg,
 [] {
 // the code here incurs
 // excessive wrapping
 // such as
 Method(
 some_med_arg,
 some_med_arg);
 some_var =
 some_expr + something;
});

Here everything is technically correct, yet the code looks bad due to excessive 
wrapping. Things look a lot better when the lambda body starts from a new line, 
which happens already in some cases. The patch this "from the next line" onto a 
few more cases.

The full discussion of the cases with @krasimir is above.

Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1268706, @djasper wrote:

> Ok, I think I agree with both of you to a certain extent, but I also think 
> this change as is, is not yet right.
>
> First, it does too much. The original very first example in this CL is 
> actually not produced by clang-format (or if it is, I don't know with which 
> flag combination). It is a case where the lambda is the last parameter.


Right, I cheated and created that example by hand. My apologies for the 
confusion. I've just pasted real code that I pumped through `clang-format`. 
Please take a look at the updated summary.

> Second, I agree that the original behavior is inconsistent in a way that we 
> have a special cases for when the lambda is the very first parameter, but 
> somehow seem be forgetting about that when it's not the first parameter. I'd 
> be ok with "fixing" that (it's not a clear-cut bug, but I think the 
> alternative behavior would be cleaner). However, I don't think your patch is 
> doing enough there. I think this should be irrespective of bin-packing (it's 
> related but not quite the same thing),

Also there is a special case for multiple lambdas. It forces line breaks. That 
aside, for the single-lambda case, are you suggesting that it is always "pulled 
up", irrespective of its place? That would contradict the direction I am trying 
to take as I like `BinPackArguments: false` and so long lamba args go to their 
own lines. This looks very much in line with what bin packing is, but not 
exactly the same. Obviously, I can add a new option `favor line breaks around 
multi-line lambda`.

Could you look at the updated summary (high level) and the new tests I added 
(low level) please? Every other test passes, so we have almost the entire spec. 
I can add a few cases where an existing snippet is reformatted with 
`BinPackArguments: false` too.

> ...and it should also apply to multiline strings if 
> AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done in 
> the same patch, but we should have a plan of what we want the end result to 
> look like and should be willing to get there.
> 
> Maybe to start with, we need the complete test matrix so that we are 
> definitely on the same page as to what we are trying to do. I imagine, we 
> would need tests for a function with three parameters, where two of the 
> parameters are short and one is a multiline lambda or a multiline string (and 
> have the lambda be the first, second and third parameter). Then we might need 
> those for both bin-packing and no-bin-packing configurations.




Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1268806, @djasper wrote:

> In https://reviews.llvm.org/D52676#1268748, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1268706, @djasper wrote:
> >
> > > Ok, I think I agree with both of you to a certain extent, but I also 
> > > think this change as is, is not yet right.
> > >
> > > First, it does too much. The original very first example in this CL is 
> > > actually not produced by clang-format (or if it is, I don't know with 
> > > which flag combination). It is a case where the lambda is the last 
> > > parameter.
> >
> >
> > Right, I cheated and created that example by hand. My apologies for the 
> > confusion. I've just pasted real code that I pumped through `clang-format`. 
> > Please take a look at the updated summary.
> >
> > > Second, I agree that the original behavior is inconsistent in a way that 
> > > we have a special cases for when the lambda is the very first parameter, 
> > > but somehow seem be forgetting about that when it's not the first 
> > > parameter. I'd be ok with "fixing" that (it's not a clear-cut bug, but I 
> > > think the alternative behavior would be cleaner). However, I don't think 
> > > your patch is doing enough there. I think this should be irrespective of 
> > > bin-packing (it's related but not quite the same thing),
> >
> > Also there is a special case for multiple lambdas. It forces line breaks. 
> > That aside, for the single-lambda case, are you suggesting that it is 
> > always "pulled up", irrespective of its place? That would contradict the 
> > direction I am trying to take as I like `BinPackArguments: false` and so 
> > long lamba args go to their own lines. This looks very much in line with 
> > what bin packing is, but not exactly the same. Obviously, I can add a new 
> > option `favor line breaks around multi-line lambda`.
>
>
> I don't think I am. You are right, there is the special case for multi-lambda 
> functions and I think we should have almost the same for single-lambda 
> functions. So, I think I agree with you and am in favor of:
>
>   someFunction(
>   a,
>   [] {
> // ...
>   },
>   b);
>   
>
> And this is irrespective of BinPacking. I think this is always better and 
> more consistent with what we'd be doing if "a" was not there. The only caveat 
> is that I think with BinPacking true or false, we should keep the more 
> compact formatting if "b" isn't there and the lambda introducer fits entirely 
> on the first line:
>
>   someFunction(a, [] {
> // ...
>   });


OK, cool, I've generalized the patch and extended the test. This global thing 
also effects other existing test that deal with sub-blocks/lambdas.

>> Could you look at the updated summary (high level) and the new tests I added 
>> (low level) please? Every other test passes, so we have almost the entire 
>> spec. I can add a few cases where an existing snippet is reformatted with 
>> `BinPackArguments: false` too.
> 
> Not sure I am seeing new test cases and I think at least a few cases are 
> missing from the entire spec, e.g. the case above.
> 
> Also, try to reduce the test cases a bit more:
> 
> - They don't need the surrounding functions
> - You can force the lambda to be multi-line with a "//" comment
> - There is no reason to have the lambda be an argument to a member function, 
> a free-standing function works just as well
> 
>   This might seem nit-picky, but in my experience, the more we can reduce the 
> test cases, the easier to read and the less brittle they become.

Makes sense, reduced the extra levels.

>>> ...and it should also apply to multiline strings if 
>>> AlwaysBreakBeforeMultilineStrings is true. It doesn't all have to be done 
>>> in the same patch, but we should have a plan of what we want the end result 
>>> to look like and should be willing to get there.

Oh, yeah, I will look at that next.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 170270.
oleg.smolsky added a comment.

Generalized the patch so that it deals with lambda args irrespective of bin 
packing. Added additional tests and patched existing ones.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3178,25 +3178,27 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
 
   verifyNoCrash("^{v^{a}}");
 }
 
 TEST_F(FormatTest, FormatNestedBlocksInMacros) {
-  EXPECT_EQ("#define MACRO() \\\n"
-"  Debug(aaa, /* force line break */ \\\n"
-"{   \\\n"
-"  int i;\\\n"
-"  int j;\\\n"
-"})",
+  EXPECT_EQ("#define MACRO()   \\\n"
+"  Debug(  \\\n"
+"  aaa, /* force line break */ \\\n"
+"  {   \\\n"
+"int i;\\\n"
+"int j;\\\n"
+"  })",
 format("#define   MACRO()   Debug(aaa,  /* force line break */ \\\n"
"  {  int   i;  int  j;   })",
getGoogleStyle()));
@@ -11596,9 +11598,10 @@
"  other(x.begin(), x.end(), [&](int, int) { return 1; });\n"
"}\n");
   verifyFormat("void f() {\n"
-   "  other(x.begin(), //\n"
-   "x.end(),   //\n"
-   "[&](int, int) { return 1; });\n"
+   "  other(\n"
+   "  x.begin(), //\n"
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
@@ -11651,9 +11654,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11680,17 +11683,74 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n",
+ Style);
+  }
+  {
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n");
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFu

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 170515.
oleg.smolsky edited the summary of this revision.
oleg.smolsky added a comment.

Added another test case.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3277,25 +3277,27 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
 
   verifyNoCrash("^{v^{a}}");
 }
 
 TEST_F(FormatTest, FormatNestedBlocksInMacros) {
-  EXPECT_EQ("#define MACRO() \\\n"
-"  Debug(aaa, /* force line break */ \\\n"
-"{   \\\n"
-"  int i;\\\n"
-"  int j;\\\n"
-"})",
+  EXPECT_EQ("#define MACRO()   \\\n"
+"  Debug(  \\\n"
+"  aaa, /* force line break */ \\\n"
+"  {   \\\n"
+"int i;\\\n"
+"int j;\\\n"
+"  })",
 format("#define   MACRO()   Debug(aaa,  /* force line break */ \\\n"
"  {  int   i;  int  j;   })",
getGoogleStyle()));
@@ -11735,9 +11737,10 @@
"  other(x.begin(), x.end(), [&](int, int) { return 1; });\n"
"}\n");
   verifyFormat("void f() {\n"
-   "  other(x.begin(), //\n"
-   "x.end(),   //\n"
-   "[&](int, int) { return 1; });\n"
+   "  other(\n"
+   "  x.begin(), //\n"
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
@@ -11790,9 +11793,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11819,17 +11822,78 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n",
+ Style);
+  }
+  {
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n");
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFunction(\n"
+   "a,\n"
+   "[t

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11604
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");

krasimir wrote:
> This looks a bit suspicious: I'd expect a break before the first arg to be 
> forced only when there exists a multiline (after formatting) lambda 
> expression arg. Is this (multiline vs. lambdas fitting 1 line) something that 
> we (can) differentiate with respect to? djasper@ might have an insight on 
> this.
Well, yes, I can see where you are coming from - the lambda is short and would 
fit. Unfortunately, I am not sure how to implement this nuance... I think I'd 
need to get the length of the unwrapped line and then check whether it fits in 
TokenUnnotator.cc

Also, I personally favor less indentation (i.e. full width for the lambda) as 
that prevents drastic reformat when the lambda body changes. (that's why this 
patch exists)



Comment at: unittests/Format/FormatTest.cpp:11657
"}});");
-  verifyFormat("virtual (std::function  
=\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; 
},\n"

krasimir wrote:
> Similar to my previous comment, this forcing the std::function on a newline 
> here might not be what we want to end up with?
I think this change is in line with the updated/extended semantics - the extra 
arg forces the lambda to its own line and the introducer is kept with the 
preceding tokens.



Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"

krasimir wrote:
> Could we please have a test case where there are several args packed on the 
> first line, then a line break, then an arg, then a multiline lambda as a last 
> arg (illustrating that we don't pull the first arg down if there's only a 
> multiline lambda as the last arg):
> ```
> function(a, b, ccc,
>  d, [] () {
>   body
> });
> ```
Sure, that seems to work, but not in the way you expected :) I'll update the 
patch...

```
  verifyFormat("function(a, b, c, //\n"
   " d, [this, that] {\n"
   "   //\n"
   " });\n");
```


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11604
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");

djasper wrote:
> oleg.smolsky wrote:
> > krasimir wrote:
> > > This looks a bit suspicious: I'd expect a break before the first arg to 
> > > be forced only when there exists a multiline (after formatting) lambda 
> > > expression arg. Is this (multiline vs. lambdas fitting 1 line) something 
> > > that we (can) differentiate with respect to? djasper@ might have an 
> > > insight on this.
> > Well, yes, I can see where you are coming from - the lambda is short and 
> > would fit. Unfortunately, I am not sure how to implement this nuance... I 
> > think I'd need to get the length of the unwrapped line and then check 
> > whether it fits in TokenUnnotator.cc
> > 
> > Also, I personally favor less indentation (i.e. full width for the lambda) 
> > as that prevents drastic reformat when the lambda body changes. (that's why 
> > this patch exists)
> I agree with Krasimir here.
> 
> If you prefer less indentation, great. Set AlignAfterOpenBracket to 
> "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested 
> before).
> 
> In more seriousness, I think getting all these cases right, I appreciate that 
> it is difficult. However, I also like to make sure that we do get them right. 
> Changing clang-format's behavior for any of these cases is not a small thing, 
> it will cause churn for *a lot* of people. We should try really hard to not 
> have changes in there that people will find detrimental. This of course is 
> subjective, so we won't get to 100%, but if in doubt for specific cases, 
> let's err on the side of not changing the current behavior.
> If you prefer less indentation, great. Set AlignAfterOpenBracket to 
> "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested 
> before).

Well, `AlignAfterOpenBracket` is too big a gun as we talking about lambda 
function args here.

> In more seriousness, I think getting all these cases right, I appreciate that 
> it is difficult. However, I also like to make sure that we do get them right. 
> Changing clang-format's behavior for any of these cases is not a small thing, 
> it will cause churn for *a lot* of people. We should try really hard to not 
> have changes in there that people will find detrimental. This of course is 
> subjective, so we won't get to 100%, but if in doubt for specific cases, 
> let's err on the side of not changing the current behavior.

OK, that's fair. Let me try to figure out how to get the former behavior 
restored for this case.



Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"

djasper wrote:
> oleg.smolsky wrote:
> > krasimir wrote:
> > > Could we please have a test case where there are several args packed on 
> > > the first line, then a line break, then an arg, then a multiline lambda 
> > > as a last arg (illustrating that we don't pull the first arg down if 
> > > there's only a multiline lambda as the last arg):
> > > ```
> > > function(a, b, ccc,
> > >  d, [] () {
> > >   body
> > > });
> > > ```
> > Sure, that seems to work, but not in the way you expected :) I'll update 
> > the patch...
> > 
> > ```
> >   verifyFormat("function(a, b, c, //\n"
> >" d, [this, that] {\n"
> >"   //\n"
> >" });\n");
> > ```
> We should try to prevent that (unless it's also the current behavior of 
> course). People have filed various bugs about this before and it is not 
> generally an accepted formatting.
This behavior is consistent with 5.0 and 6.0, so we are OK.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11604
+   "  x.end(),   //\n"
+   "  [&](int, int) { return 1; });\n"
"}\n");

oleg.smolsky wrote:
> djasper wrote:
> > oleg.smolsky wrote:
> > > krasimir wrote:
> > > > This looks a bit suspicious: I'd expect a break before the first arg to 
> > > > be forced only when there exists a multiline (after formatting) lambda 
> > > > expression arg. Is this (multiline vs. lambdas fitting 1 line) 
> > > > something that we (can) differentiate with respect to? djasper@ might 
> > > > have an insight on this.
> > > Well, yes, I can see where you are coming from - the lambda is short and 
> > > would fit. Unfortunately, I am not sure how to implement this nuance... I 
> > > think I'd need to get the length of the unwrapped line and then check 
> > > whether it fits in TokenUnnotator.cc
> > > 
> > > Also, I personally favor less indentation (i.e. full width for the 
> > > lambda) as that prevents drastic reformat when the lambda body changes. 
> > > (that's why this patch exists)
> > I agree with Krasimir here.
> > 
> > If you prefer less indentation, great. Set AlignAfterOpenBracket to 
> > "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested 
> > before).
> > 
> > In more seriousness, I think getting all these cases right, I appreciate 
> > that it is difficult. However, I also like to make sure that we do get them 
> > right. Changing clang-format's behavior for any of these cases is not a 
> > small thing, it will cause churn for *a lot* of people. We should try 
> > really hard to not have changes in there that people will find detrimental. 
> > This of course is subjective, so we won't get to 100%, but if in doubt for 
> > specific cases, let's err on the side of not changing the current behavior.
> > If you prefer less indentation, great. Set AlignAfterOpenBracket to 
> > "AlwaysBreak" and we don't need any of this ;-) (as Krasimir has suggested 
> > before).
> 
> Well, `AlignAfterOpenBracket` is too big a gun as we talking about lambda 
> function args here.
> 
> > In more seriousness, I think getting all these cases right, I appreciate 
> > that it is difficult. However, I also like to make sure that we do get them 
> > right. Changing clang-format's behavior for any of these cases is not a 
> > small thing, it will cause churn for *a lot* of people. We should try 
> > really hard to not have changes in there that people will find detrimental. 
> > This of course is subjective, so we won't get to 100%, but if in doubt for 
> > specific cases, let's err on the side of not changing the current behavior.
> 
> OK, that's fair. Let me try to figure out how to get the former behavior 
> restored for this case.
OK, I've just implemented heuristics for counting the introducer's length, but 
it turns out that it's not needed. I just had a bug in one of the conditions... 
So, I've added a couple more tests and will try posting a patch against the new 
monorepo momentarily...


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 170773.
oleg.smolsky added a comment.

Corrected test regressions.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -72,16 +72,18 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle &Style = getLLVMStyle()) {
-EXPECT_EQ(Expected.str(), format(Expected, Style))
-<< "Expected code is not stable";
+//EXPECT_EQ(Expected.str(), format(Expected, Style))
+//<< "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
+#if 0
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++
   // needs to be checked for Objective-C++ as well.
   FormatStyle ObjCStyle = Style;
   ObjCStyle.Language = FormatStyle::LK_ObjC;
   EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));
 }
+#endif
   }
 
   void verifyFormat(llvm::StringRef Code,
@@ -3277,11 +3279,12 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
@@ -11739,6 +11742,18 @@
"x.end(),   //\n"
"[&](int, int) { return 1; });\n"
"}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) { return 1; });\n"
+   "}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) {\n"
+   "//\n"
+   "  });\n"
+   "}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
"});");
@@ -11790,9 +11805,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11819,17 +11834,78 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n",
+ Style);
+  }
+  {
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n");
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments v

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:78
 EXPECT_EQ(Expected.str(), format(Code, Style));
+#if 0
 if (Style.Language == FormatStyle::LK_Cpp) {

Oops., let me fix this...


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 170776.
oleg.smolsky added a comment.

Corrected test regressions, removed temporary hacks.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3277,11 +3277,12 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
@@ -11739,6 +11740,18 @@
"x.end(),   //\n"
"[&](int, int) { return 1; });\n"
"}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) { return 1; });\n"
+   "}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) {\n"
+   "//\n"
+   "  });\n"
+   "}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
"});");
@@ -11790,9 +11803,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11819,17 +11832,78 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n",
+ Style);
+  }
+  {
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n");
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFunction(\n"
+   "a,\n"
+   "[this] {\n"
+   "  D1();\n"
+   "},\n"
+   "b);\n");
+
+  // A multi-line lambda is pulled up as long as the introducer fits on the previous
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"
+   "});\n");
+  verifyFormat("function([this, that] {\n"
+   "  //\n"
+   "});\n");
+  verifyFormat("function(a, b, c, //\n"
+   " d, [this, that] {\n"
+   "   //\n"
+   " });\n");
+
+  // Multiple lambdas are treated correctly even when there is a short 

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11736
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"

krasimir wrote:
> oleg.smolsky wrote:
> > djasper wrote:
> > > oleg.smolsky wrote:
> > > > krasimir wrote:
> > > > > Could we please have a test case where there are several args packed 
> > > > > on the first line, then a line break, then an arg, then a multiline 
> > > > > lambda as a last arg (illustrating that we don't pull the first arg 
> > > > > down if there's only a multiline lambda as the last arg):
> > > > > ```
> > > > > function(a, b, ccc,
> > > > >  d, [] () {
> > > > >   body
> > > > > });
> > > > > ```
> > > > Sure, that seems to work, but not in the way you expected :) I'll 
> > > > update the patch...
> > > > 
> > > > ```
> > > >   verifyFormat("function(a, b, c, //\n"
> > > >" d, [this, that] {\n"
> > > >"   //\n"
> > > >" });\n");
> > > > ```
> > > We should try to prevent that (unless it's also the current behavior of 
> > > course). People have filed various bugs about this before and it is not 
> > > generally an accepted formatting.
> > This behavior is consistent with 5.0 and 6.0, so we are OK.
> Maybe add a FIXME for that test that this is not ideal formatting and we 
> should also be pulling the first arg on a newline in that case too in the 
> future then.
Sure, added a comment:

```
  // FIXME: this format is not ideal and we should consider forcing the first 
arg
  // onto its own line.
  verifyFormat("function(a, b, c, //\n"
   " d, [this, that] {\n"
   "   //\n"
   " });\n");
```


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 170915.
oleg.smolsky added a comment.

Added a FIXME comment at Krasimir's request.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3277,11 +3277,12 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
@@ -11739,6 +11740,18 @@
"x.end(),   //\n"
"[&](int, int) { return 1; });\n"
"}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) { return 1; });\n"
+   "}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) {\n"
+   "//\n"
+   "  });\n"
+   "}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
"});");
@@ -11790,9 +11803,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11819,17 +11832,80 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n",
+ Style);
+  }
+  {
+verifyFormat("SomeFunction(\n"
+ "a,\n"
+ "[this] {\n"
+ "  //\n"
+ "},\n"
+ "b);\n");
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFunction(\n"
+   "a,\n"
+   "[this] {\n"
+   "  D1();\n"
+   "},\n"
+   "b);\n");
+
+  // A multi-line lambda is pulled up as long as the introducer fits on the previous
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"
+   "});\n");
+  verifyFormat("function([this, that] {\n"
+   "  //\n"
+   "});\n");
+  // FIXME: this format is not ideal and we should consider forcing the first arg
+  // onto its own line.
+  verifyFormat("function(a, b, c, //\n"
+   " d, [this, that] {\n"
+   "   //\n"
+

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

Folks, are there any other comments/suggestions?


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11854
+  // case above.
+  {
+auto Style = getGoogleStyle();

djasper wrote:
> No need to use a scope here. Feel free to redefine Style.
> 
> If in fact you feel like that is getting out of hand, maybe extract a 
> separate TEST_F() for this.
OK, sure, removed the scoped block.



Comment at: unittests/Format/FormatTest.cpp:11865
+  }
+  {
+verifyFormat("SomeFunction(\n"

djasper wrote:
> No need for this scope.
Oh, right, that's vestigial. Removed.


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 171932.
oleg.smolsky marked 2 inline comments as done.

Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3277,11 +3277,12 @@
  "});");
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 45;
-  verifyFormat("Debug(a,\n"
-   "  {\n"
-   "if () return;\n"
-   "  },\n"
-   "  a);",
+  verifyFormat("Debug(\n"
+   "a,\n"
+   "{\n"
+   "  if () return;\n"
+   "},\n"
+   "a);",
Style);
 
   verifyFormat("SomeFunction({MACRO({ return output; }), b});");
@@ -11739,6 +11740,18 @@
"x.end(),   //\n"
"[&](int, int) { return 1; });\n"
"}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) { return 1; });\n"
+   "}\n");
+  verifyFormat("void f() {\n"
+   "  other.other.other.other.other(\n"
+   "  x.begin(), x.end(),\n"
+   "  [something, rather](int, int, int, int, int, int, int) {\n"
+   "//\n"
+   "  });\n"
+   "}\n");
   verifyFormat("SomeFunction([]() { // A cool function...\n"
"  return 43;\n"
"});");
@@ -11790,9 +11803,9 @@
   verifyFormat("SomeFunction({[&] {\n"
"  // comment\n"
"}});");
-  verifyFormat("virtual (std::function  =\n"
-   " [&]() { return true; },\n"
-   " a a);");
+  verifyFormat("virtual (\n"
+   "std::function  = [&]() { return true; },\n"
+   "a a);");
 
   // Lambdas with return types.
   verifyFormat("int c = []() -> int { return 2; }();\n");
@@ -11819,17 +11832,76 @@
"  return 1; //\n"
"};");
 
-  // Multiple lambdas in the same parentheses change indentation rules.
+  // Multiple lambdas in the same parentheses change indentation rules. These
+  // lambdas are forced to start on new lines.
   verifyFormat("SomeFunction(\n"
"[]() {\n"
-   "  int i = 42;\n"
-   "  return i;\n"
+   "  //\n"
"},\n"
"[]() {\n"
-   "  int j = 43;\n"
-   "  return j;\n"
+   "  //\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("SomeFunction(\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "1);\n");
+
+  // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0
+  // case above.
+  auto Style = getGoogleStyle();
+  Style.BinPackArguments = false;
+  verifyFormat("SomeFunction(\n"
+   "a,\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "b);\n",
+   Style);
+  verifyFormat("SomeFunction(\n"
+   "a,\n"
+   "[this] {\n"
+   "  //\n"
+   "},\n"
+   "b);\n");
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value (as long as the code is wide enough).
+  verifyFormat("something->SomeFunction(\n"
+   "a,\n"
+   "[this] {\n"
+   "  D1();\n"
+   "},\n"
+   "b);\n");
+
+  // A multi-line lambda is pulled up as long as the introducer fits on the previous
+  // line and there are no further args.
+  verifyFormat("function(1, [this, that] {\n"
+   "  //\n"
+   "});\n");
+  verifyFormat("function([this, that] {\n"
+   "  //\n"
+   "});\n");
+  // FIXME: this format is not ideal and we should consider forcing the first arg
+  // onto its own line.
+  verifyFormat("function(a, b, c, //\n"
+   " d, [this, that] {\n"
+   "   //\n"
+   " });\n");
+
+  // Multiple lambdas are treated correctly even wh

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1282335, @krasimir wrote:

> Looks good! Will stamp when the scopes are removed.


Cool, thanks, Krasimir. I've just posted the updated patch.

> Oleg, do you need someone to commit this for you?

I do, could you commit this please?


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 167611.
oleg.smolsky marked an inline comment as done.
oleg.smolsky added a comment.

Tweaked if/else/return structure, added comments. No functional changes.


Repository:
  rC Clang

https://reviews.llvm.org/D52676

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11785,6 +11785,60 @@
"  return j;\n"
"});");
 
+  // A lambda passed as arg0 is always pushed to the next line.
+  verifyFormat("void f() {\n"
+   "  something->Method1(\n"
+   "  [this] {\n"
+   "Do1();\n"
+   "Do2();\n"
+   "  },\n"
+   "  1);\n"
+   "}\n");
+
+  // A lambda passed as arg1 forces arg0 to be pushed out when
+  // BinPackArguments=false.
+  {
+auto Style = getGoogleStyle();
+Style.BinPackArguments = false;
+verifyFormat("void f() {\n"
+ "  something->Method2s(\n"
+ "  1,\n"
+ "  [this] {\n"
+ "Do1();\n"
+ "Do2();\n"
+ "  },\n"
+ "  1);\n"
+ "}\n",
+ Style);
+  }
+
+  // A lambda with a very long line forces arg0 to be pushed out irrespective of
+  // the BinPackArguments value.
+  verifyFormat("void f() {\n"
+   "  something->Method2l(\n"
+   "  1,\n"
+   "  [this] {\n"
+   "Do1();\n"
+   "D01();\n"
+   "  },\n"
+   "  1);\n"
+   "}\n");
+
+  // Multiple lambdas are treated correctly even when there is a short arg0.
+  verifyFormat("void f() {\n"
+   "  something->Method3(\n"
+   "  1,\n"
+   "  [this] {\n"
+   "Do1();\n"
+   "Do2();\n"
+   "  },\n"
+   "  [this] {\n"
+   "Do1();\n"
+   "Do2();\n"
+   "  },\n"
+   "  1);\n"
+   "}\n");
+
   // More complex introducers.
   verifyFormat("return [i, args...] {};");
 
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -988,18 +988,17 @@
   Path.push_front(Best);
   Best = Best->Previous;
 }
-for (std::deque::iterator I = Path.begin(), E = Path.end();
- I != E; ++I) {
+for (auto I = Path.begin(), E = Path.end(); I != E; ++I) {
   unsigned Penalty = 0;
   formatChildren(State, (*I)->NewLine, /*DryRun=*/false, Penalty);
   Penalty += Indenter->addTokenToState(State, (*I)->NewLine, false);
 
   LLVM_DEBUG({
 printLineState((*I)->Previous->State);
 if ((*I)->NewLine) {
   llvm::dbgs() << "Penalty for placing "
-   << (*I)->Previous->State.NextToken->Tok.getName() << ": "
-   << Penalty << "\n";
+   << (*I)->Previous->State.NextToken->Tok.getName()
+   << " on a new line: " << Penalty << "\n";
 }
   });
 }
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -3043,6 +3043,22 @@
   return true;
   }
 
+  // Deal with lambda arguments in C++ - we want consistent line breaks whether
+  // they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
+  // as aggressive line breaks only make sense when the user had set
+  // Style.BinPackArguments to 'false'.
+  if ((Style.Language == FormatStyle::LK_Cpp ||
+   Style.Language == FormatStyle::LK_ObjC) &&
+  Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
+  !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
+// Always break lines in the "multiple lumbdas" case.
+if (Left.BlockParameterCount > 1)
+  return true;
+// Break lines with a lambda when the style disallows arg packing.
+if (!Style.BinPackArguments)
+  return true;
+  }
+
   return false;
 }
 
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1135,7 +1135,8 @@
   //   }, a, b, c);
   if (Current.isNot(tok::comment) && Previous &&
   Previous->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) &&
-  !Previous->is(TT_Dic

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:

> In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> >
> > > IMO `BinPackArguments==false` does not imply that there should be a line 
> > > break before the first arguments, only that there should not be two 
> > > arguments from the same argument list that appear on the same line.
> >
> >
> > That's right. However consider the following points:
> >
> > 1. a lambda function is already placed onto its own line when it is the 
> > first arg (as you can see in the first test)
>
>
> I believe that a newline before a first arg lambda is controlled by a 
> different mechanism independent of bin packing, probably by a rule that is 
> more general than just "newline before first arg lambda". I think djasper@ 
> could know more about this case.


Hmm... perhaps. I have not been able to find an explicit rule for that.

>> 1. the other args are placed onto a distinct line
>> 2. this behavior looks very close to "bin packing"
>> 3. adding a new option for the minor cases seems to be an overkill
>> 
>>   Having said that, I can add a new explicit style option. Do you think that 
>> will improve consensus? Would you expect test cases for positive and 
>> negative values of the option?
> 
> I don't see how the example before:
> 
>   void f() {
> something->Method2s(1,
> [this] {
>   Do1();
>   Do2();
> },
> 1);
>   }
> 
> 
> is inconsistent, as not adding a newline before the first argument is 
> typical, as in:
> 
>   $ clang-format -style='{BasedOnStyle: google, BinPackArguments: false}' 
> test.cc ~
>   void f() {
> something->Method2s("11",
> 
> "",
> 3);
>   }

Right, it's consistent with your example but inconsistent with with the 
following two:

lambda at arg0:

  void f() {
something->Method2s(
[this] {
  Do1();
  Do2();
},
1);
  }

and two lambdas:

  // Multiple lambdas in the same parentheses change indentation rules.
  verifyFormat("SomeFunction(\n"
   "[]() {\n"
   "  int i = 42;\n"
   "  return i;\n"
   "},\n"
   "[]() {\n"
   "  int j = 43;\n"
   "  return j;\n"
   "});");


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:

> In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
>
> > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> > >
> > > > IMO `BinPackArguments==false` does not imply that there should be a 
> > > > line break before the first arguments, only that there should not be 
> > > > two arguments from the same argument list that appear on the same line.
> > >
> > >
> > > That's right. However consider the following points:
> > >
> > > 1. a lambda function is already placed onto its own line when it is the 
> > > first arg (as you can see in the first test)
> >
> >
> > I believe that a newline before a first arg lambda is controlled by a 
> > different mechanism independent of bin packing, probably by a rule that is 
> > more general than just "newline before first arg lambda". I think djasper@ 
> > could know more about this case.
>
>
> Hmm... perhaps. I have not been able to find an explicit rule for that.
>
> >> 1. the other args are placed onto a distinct line
> >> 2. this behavior looks very close to "bin packing"
> >> 3. adding a new option for the minor cases seems to be an overkill
> >> 
> >>   Having said that, I can add a new explicit style option. Do you think 
> >> that will improve consensus? Would you expect test cases for positive and 
> >> negative values of the option?
> > 
> > I don't see how the example before:
> > 
> >   void f() {
> > something->Method2s(1,
> > [this] {
> >   Do1();
> >   Do2();
> > },
> > 1);
> >   }
> > 
> > 
> > is inconsistent, as not adding a newline before the first argument is 
> > typical, as in:
> > 
> >   $ clang-format -style='{BasedOnStyle: google, BinPackArguments: false}' 
> > test.cc ~
> >   void f() {
> > something->Method2s("11",
> > 
> > "",
> > 3);
> >   }
>
> Right, it's consistent with your example but inconsistent with with the 
> following two:
>
> lambda at arg0:
>
>   void f() {
> something->Method2s(
> [this] {
>   Do1();
>   Do2();
> },
> 1);
>   }
>
>
> and two lambdas:
>
>   // Multiple lambdas in the same parentheses change indentation rules.
>   verifyFormat("SomeFunction(\n"
>"[]() {\n"
>"  int i = 42;\n"
>"  return i;\n"
>"},\n"
>"[]() {\n"
>"  int j = 43;\n"
>"  return j;\n"
>"});");
>   




In https://reviews.llvm.org/D52676#1251064, @krasimir wrote:

> In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> > >
> > > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> > > >
> > > > > IMO `BinPackArguments==false` does not imply that there should be a 
> > > > > line break before the first arguments, only that there should not be 
> > > > > two arguments from the same argument list that appear on the same 
> > > > > line.
> > > >
> > > >
> > > > That's right. However consider the following points:
> > > >
> > > > 1. a lambda function is already placed onto its own line when it is the 
> > > > first arg (as you can see in the first test)
> > >
> > >
> > > I believe that a newline before a first arg lambda is controlled by a 
> > > different mechanism independent of bin packing, probably by a rule that 
> > > is more general than just "newline before first arg lambda". I think 
> > > djasper@ could know more about this case.
> >
> >
> > Hmm... perhaps. I have not been able to find an explicit rule for that.
> >
> > >> 1. the other args are placed onto a distinct line
> > >> 2. this behavior looks very close to "bin packing"
> > >> 3. adding a new option for the minor cases seems to be an overkill
> > >> 
> > >>   Having said that, I can add a new explicit style option. Do you think 
> > >> that will improve consensus? Would you expect test cases for positive 
> > >> and negative values of the option?
>
>
> Not really. It's generally hard to add a new style option, as that is 
> mandated by an existing commonly used public style guide that requires it.
>
> >> is inconsistent, as not adding a newline before the first argument is 
> >> typical, as in:
> >> 
> >>   $ clang-format -style='{BasedOnStyle: google, BinPackArguments: false}' 
> >> test.cc ~
> >>   void f() {
> >> something->Method2s("11",
> >>  

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In https://reviews.llvm.org/D52676#1251342, @krasimir wrote:

> Digging a bit further, seems like the behavior you're looking for could be 
> achieved by setting the `AlignAfterOpenBracket` option to `DontAlign` or 
> `AlwaysBreak`:
>
>   % clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: 
> AlwaysBreak}' test.cc
>void f() {
>  something->Method2s(
>  1,
>  [this] {
>Do1();
>Do2();
>  },
>  1);
>}
>% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket: 
> DontAlign}' test.cc
>void f() {
>  something->Method2s(1,
>  [this] {
>Do1();
>Do2();
>  },
>  1);
>}
>
> Does this work for you?


This is interesting. It obviously does what I want with the lambda, but forces 
this:

  void f() {
auto stub = PopulateContextHereAndHereSomethi(GetSomethingHere(),
  GetSomethingElseHere());
  }

into this:

  void f() {
auto stub = PopulateContextHereAndHereSomethi(
GetSomethingHere(), GetSomethingElseHere());
  }

The former looks better to me and that's what Emacs does when you press Tab. I 
think people here at work will balk at this formatting...

> I don't think modifying the behavior as posed in this change based on the 
> existence of multiline lambdas and the value of the `BinPackArguments` option 
> is better than what we have now, especially when 
> `AlignAfterOpenBracket=Align`.

Yeah, I hear you. The patch is intended to work with 
`AlignAfterOpenBracket=Align` and `BinPackArguments=false` and only tweaks the 
lambdas' placement.

How about I key the new behavior off a new value of `AlignAfterOpenBracket`, 
such as `AlignAfterOpenBracket=AlignExceptLambda`?


Repository:
  rC Clang

https://reviews.llvm.org/D52676



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

[PATCH] D52676: [clang-format] tweaked another case of lambda formatting

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

@MyDeveloperDay that's the exact point. I authored this change to close a gap 
in some lambda formatting cases. The tests (existing, modified and added) 
express all relevant cases that I knew at the time.

@jaafar, @curdeius - this is just C++ code. Nothing is set in stone. Of course 
there are other cases that are effected by this change. The right way forward 
here is for you guys to author a new change. Ideally, the change will bring the 
new test, format it to your liking and change nothing else.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52676

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky created this revision.
Herald added a subscriber: carlosgalvezp.
oleg.smolsky requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

- we want to deal with non-default constructors that just happen to contain 
constant initializers

There was already a negative test case, it is now a positive one. We find and 
refactor this case:

  struct PositiveNotDefaultInt {
PositiveNotDefaultInt(int) : i(7) {}
int i;
  };


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  test/clang-tidy/checkers/modernize-use-default-member-init.cpp


Index: test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,15 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +243,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -213,7 +213,6 @@
 
   Finder->addMatcher(
   cxxConstructorDecl(
-  isDefaultConstructor(),
   forEachConstructorInitializer(
   cxxCtorInitializer(
   forField(unless(anyOf(getLangOpts().CPlusPlus20


Index: test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,15 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +243,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -213,7 +213,6 @@
 
   Finder->addMatcher(
   cxxConstructorDecl(
-  isDefaultConstructor(),
   forEachConstructorInitializer(
   cxxCtorInitializer(
   forField(unless(anyOf(getLangOpts().CPlusPlus20
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 391470.
oleg.smolsky added a comment.

Fixing the uploaded diff...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,38 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt
+{
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2
+{
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), 
j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int 
arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +266,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -213,7 +213,6 @@
 
   Finder->addMatcher(
   cxxConstructorDecl(
-  isDefaultConstructor(),
   forEachConstructorInitializer(
   cxxCtorInitializer(
   forField(unless(anyOf(getLangOpts().CPlusPlus20


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,38 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt
+{
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2
+{
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +266,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -213,7 +213,6 @@
 
   Finder->addMatcher(
   cxxConstructorDecl(
-  isDefaultConstructor(),
   forEachConstructorInitializer(
   cxxCtorInitializer(
   forField(unless(anyOf(getLangOpts().CPlusPlus20
___
cfe-commits mailing list
cfe-commit

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 391494.
oleg.smolsky added a comment.

Ran `clang-format`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,38 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt
+{
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2
+{
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), 
j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int 
arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +266,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -212,17 +212,14 @@
 InitBase);
 
   Finder->addMatcher(
-  cxxConstructorDecl(
-  isDefaultConstructor(),
-  forEachConstructorInitializer(
-  cxxCtorInitializer(
-  forField(unless(anyOf(getLangOpts().CPlusPlus20
-? unless(anything())
-: isBitField(),
-hasInClassInitializer(anything()),
-hasParent(recordDecl(isUnion()),
-  withInitializer(Init))
-  .bind("default"))),
+  cxxConstructorDecl(forEachConstructorInitializer(
+  cxxCtorInitializer(
+  forField(unless(anyOf(
+  getLangOpts().CPlusPlus20 ? unless(anything()) : 
isBitField(),
+  hasInClassInitializer(anything()),
+  hasParent(recordDecl(isUnion()),
+  withInitializer(Init))
+  .bind("default"))),
   this);
 
   Finder->addMatcher(


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,38 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt
+{
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt
+{
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2
+{
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +266,6

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 391503.
oleg.smolsky added a comment.

Ran `clang-format` on the test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,35 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt {
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt {
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2 {
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer 
for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), 
j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int 
arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +263,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -212,17 +212,14 @@
 InitBase);
 
   Finder->addMatcher(
-  cxxConstructorDecl(
-  isDefaultConstructor(),
-  forEachConstructorInitializer(
-  cxxCtorInitializer(
-  forField(unless(anyOf(getLangOpts().CPlusPlus20
-? unless(anything())
-: isBitField(),
-hasInClassInitializer(anything()),
-hasParent(recordDecl(isUnion()),
-  withInitializer(Init))
-  .bind("default"))),
+  cxxConstructorDecl(forEachConstructorInitializer(
+  cxxCtorInitializer(
+  forField(unless(anyOf(
+  getLangOpts().CPlusPlus20 ? unless(anything()) : 
isBitField(),
+  hasInClassInitializer(anything()),
+  hasParent(recordDecl(isUnion()),
+  withInitializer(Init))
+  .bind("default"))),
   this);
 
   Finder->addMatcher(


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,35 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt {
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+struct PositiveNotDefaultOOLInt {
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2 {
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky created this revision.
oleg.smolsky added a reviewer: aaron.ballman.
Herald added subscribers: carlosgalvezp, mgorny.
Herald added a project: All.
oleg.smolsky requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

- this adds a new check: `readability-return-with-redundant-parens`

We can find these with the AST matcher. We exclude macros.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
@@ -0,0 +1,69 @@
+//===--- ReturnWithRedundantParensCheck.cpp - clang-tidy *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReturnWithRedundantParensCheck.h"
+#include "clang/Lex/Lexer.h"
+#include 
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+bool isLocationInMacroExpansion(const SourceManager &SM, SourceLocation Loc) {
+  return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
+}
+} // namespace
+
+void ReturnWithRedundantParensCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  functionDecl(
+  isDefinition(), returns(unless(voidType())),
+  hasBody(compoundStmt(hasAnySubstatement(returnStmt(has(expr()
+  .bind("return"))),
+  this);
+}
+
+void ReturnWithRedundantParensCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Block = Result.Nodes.getNodeAs("return");
+  CompoundStmt::const_reverse_body_iterator Last = Block->body_rbegin

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 453084.
oleg.smolsky added a comment.

Drop the trailing \n from the main C++ file


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
@@ -0,0 +1,69 @@
+//===--- ReturnWithRedundantParensCheck.cpp - clang-tidy *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReturnWithRedundantParensCheck.h"
+#include "clang/Lex/Lexer.h"
+#include 
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+bool isLocationInMacroExpansion(const SourceManager &SM, SourceLocation Loc) {
+  return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
+}
+} // namespace
+
+void ReturnWithRedundantParensCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  functionDecl(
+  isDefinition(), returns(unless(voidType())),
+  hasBody(compoundStmt(hasAnySubstatement(returnStmt(has(expr()
+  .bind("return"))),
+  this);
+}
+
+void ReturnWithRedundantParensCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Block = Result.Nodes.getNodeAs("return");
+  CompoundStmt::const_reverse_body_iterator Last = Block->body_rbegin();
+  if (const auto *Return = dyn_cast(*Last)) {
+SourceManager &SM = *Result.SourceManager;
+
+if (isLocationInMacroExpansion(SM, Return->getSourceRange().getBegin()))
+  return;
+
+assert(!Return->children().empty());
+auto ChildExpr = *Retur

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 453151.
oleg.smolsky added a comment.

Add docs.


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+=
+
+Checks for `return` statements with redundant parenthesis.
+
+Example
+---
+
+.. code-block:: c++
+
+  void f() {
+return (1);  // Note, the parenthesis here are redundant.
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -353,6 +353,7 @@
`readability-redundant-smartptr-get `_, "Yes"
`readability-redundant-string-cstr `_, "Yes"
`readability-redundant-string-init `_, "Yes"
+   `readability-return-with-redundant-parens `_, "Yes"
`readability-simplify-boolean-expr `_, "Yes"
`readability-simplify-subscript-expr `_, "Yes"
`readability-static-accessed-through-instance `_, "Yes"
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
@@ -0,0 +1,68 @@
+//===--- ReturnWithRedundantParensCheck.cpp - clang-tidy *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois 

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 453155.
oleg.smolsky added a comment.

Add a release note.


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+=
+
+Checks for `return` statements with redundant parenthesis.
+
+Example
+---
+
+.. code-block:: c++
+
+  void f() {
+return (1);  // Note, the parenthesis here are redundant.
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -353,6 +353,7 @@
`readability-redundant-smartptr-get `_, "Yes"
`readability-redundant-string-cstr `_, "Yes"
`readability-redundant-string-init `_, "Yes"
+   `readability-return-with-redundant-parens `_, "Yes"
`readability-simplify-boolean-expr `_, "Yes"
`readability-simplify-subscript-expr `_, "Yes"
`readability-static-accessed-through-instance `_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-return-with-redundant-parens
+  ` check.
+
+  Checks for `return` statements with redundant parenthesis.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif /

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In D131985#3727436 , @njames93 wrote:

> The idea of this check is good, but restricting it to only return statements 
> seems baffling. A general check that could remove useless parens would have a 
> lot more value.

Of course a more general check would be more generally useful. Yet that 
requires a lot more code as handling many more contexts in involved in C++.

Basically this change addresses a concrete, somewhat wide-spread silly habit 
that exists in the wild.


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

https://reviews.llvm.org/D131985

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

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 453157.
oleg.smolsky added a comment.

Improve mark up in the docs.


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+
+
+Checks for ``return`` statements with redundant parenthesis.
+
+Example
+---
+
+.. code-block:: c++
+
+  void f() {
+return (1);  // Note, the parenthesis here are redundant.
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -353,6 +353,7 @@
`readability-redundant-smartptr-get `_, "Yes"
`readability-redundant-string-cstr `_, "Yes"
`readability-redundant-string-init `_, "Yes"
+   `readability-return-with-redundant-parens `_, "Yes"
`readability-simplify-boolean-expr `_, "Yes"
`readability-simplify-subscript-expr `_, "Yes"
`readability-static-accessed-through-instance `_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-return-with-redundant-parens
+  ` check.
+
+  Checks for ``return`` statements with redundant parenthesis.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_RETURNBRACKETSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find and remove redundant parenthesis surrounding returned expression.
+///
+/// Examples:
+/// \code
+///   void f() { return (1); } ==> void f() { return 1; }
+/// \endcode
+class ReturnWithRedundantParensCheck : public ClangTidyCheck {
+public:
+  ReturnWithRedundantParensCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} //

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 3 inline comments as done.
oleg.smolsky added a comment.

Thanks for the quick review, Eugene!




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:110
+
+  Checks for `return` statements with redundant parenthesis.
+

Eugene.Zelenko wrote:
> Please use double back-ticks for language constructs.
Done.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst:4
+readability-return-with-redundant-parens
+=
+

Eugene.Zelenko wrote:
> Please make same length as check name.
Done



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst:6
+
+Checks for `return` statements with redundant parenthesis.
+

Eugene.Zelenko wrote:
> Please use double back-ticks for language constructs.
Done.


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

https://reviews.llvm.org/D131985

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

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky marked 3 inline comments as done.
oleg.smolsky added a comment.

In D131985#3727568 , @Eugene.Zelenko 
wrote:

> In D131985#3727544 , @oleg.smolsky 
> wrote:
>
>> In D131985#3727436 , @njames93 
>> wrote:
>>
>>> The idea of this check is good, but restricting it to only return 
>>> statements seems baffling. A general check that could remove useless parens 
>>> would have a lot more value.
>>
>> Of course a more general check would be more generally useful. Yet that 
>> requires a lot more code as handling many more contexts in involved in C++.
>>
>> Basically this change addresses a concrete, somewhat wide-spread silly habit 
>> that exists in the wild.
>
> I recently observed in the wild patterns like:
>
>   std::vector data;
>   
>   for (std::vector::iterator it = data.begin(); it != 
> data.end(); ++it)
> std::cout << (*it) << std::endl;

While I have not seen that one, I've seen something similar, with even more 
involved syntax:

  for (auto it = data.begin(); it != data.end(); ++it)
 std::cout << (*it).something << std::endl;

people write wild syntax...


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

https://reviews.llvm.org/D131985

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

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 453740.
oleg.smolsky added a comment.

Add AST matchers for the `if` statements and improve the actual check 
implementation.


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+bool helper();
+
+bool if1(int *p) {
+  if (p) return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) return helper();{{$}}
+}
+
+bool if2(int *p) {
+  if (p) p = nullptr; else return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) p = nullptr; else return helper();{{$}}
+}
+
+bool if3(int *arg) {
+  if (arg) {
+return(helper());
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+// CHECK-FIXES: {{^ +}}return helper();{{$}}
+  } else {
+return(helper());
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+// CHECK-FIXES: {{^ +}}return helper();{{$}}
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+
+
+Checks for ``return`` statements with redundant parenthesis.
+
+Example
+---
+
+.. code-block:: c++
+
+  void f() {
+return (1);  // Note, the parenthesis here are redundant.
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -353,6 +353,7 @@
`readability-redundant-smartptr-get `_, "Yes"
`readability-redundant-string-cstr `_, "Yes"
`readability-redundant-string-init `_, "Yes"
+   `readability-return-with-redundant-parens `_, "Yes"
`readability-simplify-boolean-expr `_, "Yes"
`readability-simplify-subscript-expr `_, "Yes"
`readability-static-accessed-through-instance `_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-return-with-redundant-parens
+  ` check.
+
+  Checks for ``return`` statements with redundant parenthesis.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
@@ -0,0 +1,38 @@
+//===--- ReturnWithRedundantParensCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The L

[PATCH] D131985: clang-tidy: strip useless parens from `return` statements

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 454149.
oleg.smolsky added a comment.

Simplify and generalize the AST matcher. We just want to find all `return` 
statements in function definitions.


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

https://reviews.llvm.org/D131985

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.cpp
  clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
  clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp

Index: clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-return-with-redundant-parens.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s readability-return-with-redundant-parens %t
+
+int good() {
+  return 1;
+}
+
+int simple1() {
+  return (1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+int complex1() {
+  int a = 0;
+  return (a + a * (a + a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return a + a * (a + a);{{$}}
+}
+
+int no_space() {
+  return(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}return 1;{{$}}
+}
+
+bool helper();
+
+bool if1(int *p) {
+  if (p) return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) return helper();{{$}}
+}
+
+bool if2(int *p) {
+  if (p) p = nullptr; else return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^  }}if (p) p = nullptr; else return helper();{{$}}
+}
+
+bool if3(int *arg) {
+  if (arg) {
+return(helper());
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+// CHECK-FIXES: {{^ +}}return helper();{{$}}
+  } else {
+return(helper());
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+// CHECK-FIXES: {{^ +}}return helper();{{$}}
+  }
+
+  if (arg) {
+if (helper()) {
+  return(helper());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Redundant parens in the 'return' statement [readability-return-with-redundant-parens]
+  // CHECK-FIXES: {{^ +}}return helper();{{$}}
+}
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/return-with-redundant-parens.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-return-with-redundant-parens
+
+readability-return-with-redundant-parens
+
+
+Checks for ``return`` statements with redundant parenthesis.
+
+Example
+---
+
+.. code-block:: c++
+
+  void f() {
+return (1);  // Note, the parenthesis here are redundant.
+  }
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -353,6 +353,7 @@
`readability-redundant-smartptr-get `_, "Yes"
`readability-redundant-string-cstr `_, "Yes"
`readability-redundant-string-init `_, "Yes"
+   `readability-return-with-redundant-parens `_, "Yes"
`readability-simplify-boolean-expr `_, "Yes"
`readability-simplify-subscript-expr `_, "Yes"
`readability-static-accessed-through-instance `_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-return-with-redundant-parens
+  ` check.
+
+  Checks for ``return`` statements with redundant parenthesis.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/readability/ReturnWithRedundantParensCheck.h
===

[PATCH] D143375: clang-tidy: Count template constructors in modernize-use-default-member-init

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

Nice!


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

https://reviews.llvm.org/D143375

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 397086.
oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added a comment.

Addressed review comments: explicit types and doc changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-use-default-member-init.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,42 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt {
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+// We cannot reconcile these initializers.
+struct TwoConstructors {
+TwoConstructors(int) : i(7) {}
+TwoConstructors(int, int) : i(8) {}
+int i;
+};
+
+struct PositiveNotDefaultOOLInt {
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2 {
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +270,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-default-member-init.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-use-default-member-init.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-default-member-init.rst
@@ -3,7 +3,7 @@
 modernize-use-default-member-init
 =
 
-This check converts a default constructor's member initializers into the new
+This check converts constructors' member initializers into the new
 default member initializers in C++11. Other member initializers that match the
 default member initializer are removed. This can reduce repeated code or allow
 use of '= default'.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -73,6 +73,9 @@
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
+- Generalized the `modernize-use-default-member-init` check to handle non-default
+  constructors.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -212,17 +212,14 @@
 InitBase);
 
   Finder->addMatcher(
-  cxxConstructorDecl(
-  isDefaultConstructor(),
-  forEachConstructorInitializer(
-  cxxCtorInitializer(
-  forField(unless(anyOf(getLangOpts().CPlusPlus20
-? unless(anything())
-: isBitField(),
-hasInClassInitializer(anything()),
-hasParent(recordDecl(isUnion()),
-  withInitializer(Init))
-  .bind("default"))),
+  cxxConstructorDecl(forEachConstructorInitializer(
+  cxxCtorInitializer(
+  forField(unless(anyOf(
+  getLangOpts().CPlusPlus20 ? unless(anything()) : isBitField(),
+  hasInClassInitializer(anything()),
+  hasParent(recordDecl(isUnion()),
+  withInitializer(Init))
+  .bind("default"

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

Done with review changes.




Comment at: 
clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:1
+
 //===--- UseDefaultMemberInitCheck.cpp - 
clang-tidy===//

aaron.ballman wrote:
> Spurious newline?
yep, removing it...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

@aaron.ballman could you commit this change please? I've never had commit 
rights... Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In D114995#3218022 , @aaron.ballman 
wrote:

> In D114995#3218004 , @oleg.smolsky 
> wrote:
>
>> @aaron.ballman could you commit this change please? I've never had commit 
>> rights... Thanks!
>
> Sure can! Is `Oleg Smolsky ` the correct attribution 
> for the patch, or would you like me to use a different name or email address?

Thanks Aaron! Yes, everything here looks right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a subscriber: aaron.ballman.
oleg.smolsky added a comment.

@aaron.ballman do you happen to know who is best to review a `clang-tidy` fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

Sure, adding an option is easy, if that's the consensus. What would you call it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky updated this revision to Diff 395190.
oleg.smolsky added a comment.

Added a "two constructors" test case along with the support for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
@@ -45,6 +45,42 @@
   // CHECK-FIXES: int j{1};
 };
 
+struct PositiveNotDefaultInt {
+  PositiveNotDefaultInt(int) : i(7) {}
+  // CHECK-FIXES: PositiveNotDefaultInt(int)  {}
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+// We cannot reconcile these initializers.
+struct TwoConstructors {
+TwoConstructors(int) : i(7) {}
+TwoConstructors(int, int) : i(8) {}
+int i;
+};
+
+struct PositiveNotDefaultOOLInt {
+  PositiveNotDefaultOOLInt(int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+};
+
+PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int) : i(7) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt::PositiveNotDefaultOOLInt(int)  {}
+
+struct PositiveNotDefaultOOLInt2 {
+  PositiveNotDefaultOOLInt2(int, int);
+  int i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'i'
+  // CHECK-FIXES: int i{7};
+  int j;
+};
+
+PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) : i(7), j(arg) {}
+// CHECK-FIXES: PositiveNotDefaultOOLInt2::PositiveNotDefaultOOLInt2(int, int arg) :  j(arg) {}
+
 struct PositiveUnaryMinusInt {
   PositiveUnaryMinusInt() : j(-1) {}
   // CHECK-FIXES: PositiveUnaryMinusInt()  {}
@@ -234,12 +270,6 @@
   int i : 5;
 };
 
-struct NegativeNotDefaultInt
-{
-  NegativeNotDefaultInt(int) : i(7) {}
-  int i;
-};
-
 struct NegativeDefaultArg
 {
   NegativeDefaultArg(int i = 4) : i(i) {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -1,3 +1,4 @@
+
 //===--- UseDefaultMemberInitCheck.cpp - clang-tidy===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
@@ -212,17 +213,14 @@
 InitBase);
 
   Finder->addMatcher(
-  cxxConstructorDecl(
-  isDefaultConstructor(),
-  forEachConstructorInitializer(
-  cxxCtorInitializer(
-  forField(unless(anyOf(getLangOpts().CPlusPlus20
-? unless(anything())
-: isBitField(),
-hasInClassInitializer(anything()),
-hasParent(recordDecl(isUnion()),
-  withInitializer(Init))
-  .bind("default"))),
+  cxxConstructorDecl(forEachConstructorInitializer(
+  cxxCtorInitializer(
+  forField(unless(anyOf(
+  getLangOpts().CPlusPlus20 ? unless(anything()) : isBitField(),
+  hasInClassInitializer(anything()),
+  hasParent(recordDecl(isUnion()),
+  withInitializer(Init))
+  .bind("default"))),
   this);
 
   Finder->addMatcher(
@@ -248,6 +246,13 @@
 const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) {
   const FieldDecl *Field = Init->getAnyMember();
 
+  // Check whether we have multiple hand-written constructors and bomb out, as
+  // it is hard to reconcile their sets of member initializers.
+  auto ClassDecl = dyn_cast(Field->getParent());
+  if (std::count_if(ClassDecl->ctors().begin(), ClassDecl->ctors().end(),
+[](const auto Ctor) { return !Ctor->isCopyOrMoveConstructor(); }) > 1)
+return;
+
   SourceLocation StartLoc = Field->getBeginLoc();
   if (StartLoc.isMacroID() && IgnoreMacros)
 return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114995: clang-tidy: improve the 'modernize-use-default-member-init'

[Oleg Smolsky via Phabricator via cfe-commits]

oleg.smolsky added a comment.

In D114995#3198033 , @aaron.ballman 
wrote:

> In D114995#3183240 , 
> @malcolm.parsons wrote:
>
>> In D114995#3180475 , 
>> @aaron.ballman wrote:
>>
>>> was there a reason we didn't cover that case originally or was it an 
>>> oversight/left for future work?
>>
>> It was left for future work - by only considering the initializer list of 
>> the default constructor, clang-tidy did not have to work out what to do when 
>> the constructors don't agree on what value the member init should have.
>
> Thank you for verifying! @oleg.smolsky -- this would be a very useful test 
> case to add, btw.

Yep, done.

PLAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114995

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