Sorry for being a bit late here and thanks @klimek for bringing this to my attention.
This has been years ago, but if I reconstruct my thinking (and look at the test cases), then I'd say that "left" alignment should not be applied to multi-variable decl statements ever.

Generally, upload patches with the full file as context (that will prevent Phabricator's "Context not available")
But this change looks good. Thank you.

Looks good. Thank you.

Thank you.

Ok, but this behavior is still intended. You are setting clang-format to a format where it is breaking after binary operators and then added a break before a binary operator. clang-format assumes that this is not intended and that you will want this cleaned up.

Look at getGoogleStyle(). It has a bunch of language-specific configs at the bottom. You can do the same for TableGen in getLLVMStyle().

This seem to conceptually be a list of things rather than an array subscript, though, right? Could we alternatively set SpacesInContainerLiterals to false for LK_TableGen?

Without understanding this in more detail I do not know how to move forward with this. What this patch is describing is what should already be the case with ColumnLimit set to zero. If it isn't this might be a bug or there might be a different way to move forward. However, the flag as is does not make sense to me without more information.

I don't quite understand. What you are describing should already be the behavior with ColumnLimit=0 and I think your test should pass without the new option. Doesn't it?

Does this also work for _asm and __asm?

I think this roughly looks fine. Krasimir, any thoughts?

In D52676#1268748, @oleg.smolsky wrote:

In 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.

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.

Looks good.

I don't have very strong opinions here and I agree that we will need to improve the conditional formatting, especially as this kind of ternary-chaining is becoming more popular because of constexpr.

Ok, so IIUC, understanding that @end effective ends a section much like "}" would address the currently observed problems?

In my opinion, this only addresses one edge case where clang-format -lines output is not identical with a full reformatting. I believe they cannot all usefully be avoided. As such, I am unsure that this option carries its weight of making the implementation more complex.
How often does this happen for you? Why can't you just format the additional incorrect line?

Could you explain what problem this is fixing?

Looks good.

You are right that this behavior is what the code authors, but also many other people, like to have and so it is what is engrained in clang-format. There are likely about a million things that fall into the same category. Now we might find that the current default is actually wrong and we have changed many defaults in the past. I don't believe that's the case here and there are more opinions on this thread.

Looks good. Sorry for the delay.

The normal rule for formatting options apply. If you can dig up a public style guide and a project of reasonable size where it is used, we can add an option.

Generally looks good.

Looks good, thank you.

Looks good.

Looks good.

I understand that, but the test example does not break after the closing paren. It breaks after the subsequent "<".

Both patch and comment inside patch say "break after closing paren" and yet that's not what the test or example in the description actually do. Why is that?

Looks good.

Looks good.

Ok, you know the ObjC community much better than I do. If you think that adding the indentation for ObjC irrespective of the option works for most people, I am happy to go with it.

Please read:

I'd go to great lengths to avoid adding new config options and so I don't think this would be a bad trade-off to make.

Looks good, thank you!

I still really believe that these config options do no make sense and are actively confusing.

Looks good. I like option 2 :).

Looks good.

Looks good.

Well, I disagree. It says: "If you break after the return type of a function declaration or definition, do not indent."

Do we have a public style guide that explicitly says to do this?
That's a requirement for new style options as per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options.

Yeah, it's one of these things where neither way would be totally intuitive to everyone.

Looks good.

Generally looks good, one minor simplification.

Looks good, thank you!

Some last comments, but basically looks good.

Generally looks good.

Looks good.

Looks good.

Please generally upload diffs with the full file as context so that Phabricator can properly expand the context where necessary.

I'd really like to not parse include/import statements this way. Can you send me examples of headers where we fail to recognize them as ObjC and this matters (happy for you to send me examples offline).

Ok, looks good.

Nice.. Removed a lot of complexity :). Let's see whether this heuristic is good enough.

Right. So the difference is that for blocks, there is always a "(" after the "(^.....)". That should be easy to parse, no?

Would it be enough to only add the block type case? With the macro false positive, there won't be an open paren after the closing paren, right?

We have talked about that and none of us agree.

In D42787#1025117, @Typz wrote:

If people don't care about this case, we might as well merge this :-) Just kidding.

The tweak matches both our expectation, the auto-indent behaviour of IDE (not to be trusted, but still probably of 'default' behaviour for many people, esp. when you don't yet use a formatter), and its seems our code base is generally formatted like this. I think it is quite more frequent than 50 times in whole LLVM/Clang, but I have no actual numbers; do you have a magic tool to search for such specific "code pattern", or just run clang-format with one option then the next and count the differences ?

Got two responses so far.

Are you sure that you are even addressing an important case? I have done some research on our codebase and this is something that happens incredibly rarely. The reason is that you have to have a very specific combination of line length, where the last parameter does not fit on one line if indented back to align with the open paren while it does fit on multiple lines if indented right of the comma. In all of LLVM/Clang, there seem to be only about 50 cases. How certain are you that people actually care?

In D42684#1022219, @Typz wrote:

Indeed, seems to apply to classes as well. Maybe I was mislead by my testing, where I did not get the case (possibly because we use ConstructorInitializerAllOnOneLineOrOnePerLine=true, so the continuation indenter only sees "short" class declarations unless breaking the template is required because the name is too long).

Ah, I thought it was somehow possible to create: