Page MenuHomePhabricator

oleg.smolsky (Oleg Smolsky)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 28 2018, 4:07 PM (37 w, 4 d)

Recent Activity

Feb 6 2019

oleg.smolsky added a comment to D57770: [clang-tidy] Fixed a std::bind() transformation.

Sure, I'm OK with the license change. Please go ahead and commit the fix.

Feb 6 2019, 8:39 AM

Feb 5 2019

oleg.smolsky added a comment to D57770: [clang-tidy] Fixed a std::bind() transformation.

Jonas, could you commit the fix please? (I don't have the rights)

Feb 5 2019, 11:04 AM
oleg.smolsky added a comment to D57770: [clang-tidy] Fixed a std::bind() transformation.

:) I thought it would be more productive to post a fix than file a bug.

Feb 5 2019, 11:02 AM
oleg.smolsky created D57770: [clang-tidy] Fixed a std::bind() transformation.
Feb 5 2019, 10:56 AM

Oct 31 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

Looks good! Will stamp when the scopes are removed.

Oct 31 2018, 8:44 AM · Restricted Project
oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.
Oct 31 2018, 8:41 AM · Restricted Project
oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 31 2018, 8:39 AM · Restricted Project

Oct 26 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

Folks, are there any other comments/suggestions?

Oct 26 2018, 9:17 AM · Restricted Project

Oct 24 2018

oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

Added a FIXME comment at Krasimir's request.

Oct 24 2018, 9:33 AM · Restricted Project
oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 24 2018, 9:29 AM · Restricted Project

Oct 23 2018

oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

Corrected test regressions, removed temporary hacks.

Oct 23 2018, 4:07 PM · Restricted Project
oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 23 2018, 4:03 PM · Restricted Project
oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

Corrected test regressions.

Oct 23 2018, 4:02 PM · Restricted Project
oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 23 2018, 3:55 PM · Restricted Project
oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 23 2018, 10:58 AM · Restricted Project

Oct 22 2018

oleg.smolsky added inline comments to D52676: [clang-format] tweaked another case of lambda formatting.
Oct 22 2018, 3:34 PM · Restricted Project
oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

Added another test case.

Oct 22 2018, 3:34 PM · Restricted Project

Oct 19 2018

oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

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

Oct 19 2018, 3:44 PM · Restricted Project
oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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, [] {
  // ...
});
Oct 19 2018, 3:39 PM · Restricted Project

Oct 18 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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.

Oct 18 2018, 4:08 PM · Restricted Project
oleg.smolsky updated the summary of D52676: [clang-format] tweaked another case of lambda formatting.
Oct 18 2018, 3:51 PM · Restricted Project

Oct 10 2018

oleg.smolsky updated the summary of D52676: [clang-format] tweaked another case of lambda formatting.
Oct 10 2018, 10:39 AM · Restricted Project
oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

@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:

Oct 10 2018, 10:36 AM · Restricted Project

Oct 1 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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?

Oct 1 2018, 11:31 AM · Restricted Project
oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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("111111111111111111",
                      "2222222222222222222222222222222222222222222222222222",
                      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"
             "    });");
Oct 1 2018, 9:15 AM · Restricted Project

Sep 29 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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.

Sep 29 2018, 9:36 AM · Restricted Project
oleg.smolsky updated the diff for D52676: [clang-format] tweaked another case of lambda formatting.

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

Sep 29 2018, 9:25 AM · Restricted Project
oleg.smolsky updated the summary of D52676: [clang-format] tweaked another case of lambda formatting.
Sep 29 2018, 9:16 AM · Restricted Project

Sep 28 2018

oleg.smolsky added a comment to D52676: [clang-format] tweaked another case of lambda formatting.

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.

Sep 28 2018, 6:57 PM · Restricted Project
oleg.smolsky created D52676: [clang-format] tweaked another case of lambda formatting.
Sep 28 2018, 4:10 PM · Restricted Project