Nice trick! :)

In D102706#2823087, @vlovich wrote:

I don't think I have permissions. Happy to do it if I'm given permissions (I'm assuming the instructions are the general LLVM ones). Otherwise:

Name: Vitali Lovich
E-mail: vlovich@gmail.com

In D104096#2820206, @skirkovski wrote:

Ping,

I do not have commit access, can someone submit this as time permits ?

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

Could you split this into two diffs? Or are those two changes in any way related?

In D104044#2812399, @darwin wrote:

About the issue, let me explain it. It isn't bound to the google style or LLVM style either, since both of them keep the first brace at the same line of the namespace.

Then I would like to use the LLVM style in the tests, otherwise it suggests that the issue is a result of using google style.

Wouldn't this suggest that this issue is a result of using LLVM style?

Thanks for the work!

Can you also add tests with the alignment of declarations? We already have such for pointers.

Going the full way, to fix the number of empty lines after/before/between elements would be real nice. But even nicer would be if you can state a range.

In D99840#2718959, @curdeius wrote:

LGTM. I also consider it a bug. LLVM should not be affected as it uses AllowShortEnumsOnASingleLine: true whereas this problem arises only with AllowShortEnumsOnASingleLine: false.
Anyway, those that find the new behaviour problematic, can always set BreakBeforeBraces: Custom and BraceWrapping.AfterEnum: true to get the old behaviour.
@lunasorcery, please update release notes before landing.
If you don't have commit rights, please provide "Your Name <email@address>" for commit attribution, otherwise we'll use the name/email associated with you Phabricator account.

Then we can push it, do you need some one to do that? If yes please post name and email.

In D102706#2787550, @HazardyKnusperkeks wrote:

LGTM, but please wait for more responses.

Do you need some one to commit this? If yes please state name and email, some one will chime in to commit it.

In D101868#2810452, @feg208 wrote:

If I can get someone to submit this on my behalf I think we can call it a day.

I think this would be a nice addition, and in the past I would have used it. I really like the option Pointer.

In D104044#2810909, @darwin wrote:

Sorry, I may need some help here. It shows "Context not available.", how do I correct it?

Remains the issue with the alignment, I would like to know @MyDeveloperDay 's opinion on that. Should the values be right aligned, or left aligned? As far as I see all alignment in clang-format is left until now.

Seems reasonable. :)

I've added a few comments, and I would like to hear the opinion of others regarding the left or right alignment of the elements.

Will do on Thursday.

Under "Related Objects" you can add the commit, so that one can navigate to it.

Forgot about that: Please add an entry in the ReleaseNotes.rst, I can imagine there are some people out there waiting for this.

Looks good to me, I would just change the wording a bit. Could you please also add a entry in the ReleaseNotes.rst?

Thank you very much. :)
Do you have commit access, or do you need someone to land it? If the latter please state name and email for the commit.

Love it!

Please just check continue, I would like to make it a separate commit, because it seems unrelated to me. Otherwise this is good.

Looks quite solid for me.

LGTM, but please wait for more responses.

Although verifyFormat is nice, I would add some EXPECT_EQ to show that the braces are really inserted.

In D101868#2777423, @feg208 wrote:

In D101868#2776633, @HazardyKnusperkeks wrote:

In D101868#2775550, @feg208 wrote:

This reworks substantially this commit. I recognize there are lacking/broken
tests but I just would like to ensure that the general direction doesn't
seem likely to end in tears

I'm not deep enough in clang-format and currently have not enough time to check that in depth. But why are you right aligning?

So the basic approach here is to add the column aligning at the point that the lines are broken to make sure that we can align to the indented, now broken, columns. The right alignment was the easier path (at the moment) since the spaces attached to tokens in the change list proceeded the token and since I was calculating the column size with spaces based on a pointer attached to the token in the same column. To left align at each column I'd need to look at the adjacent column to determine the right number of spaces.

I would love to have a right aligning for numbers (or even better aligning around the decimal dot) in all my code, but strings I wouldn't want to right align.

I think we could certainly do that. I guess at this point (and this is somewhat motivated by the fact that I don't, and probably shouldn't, have commit rights the to llvm repo) I think if we want to move this commit forward we ought to agree on a set of changes and subsequent tests that we can all be comfortable with that would make this a viable bit of code. I don't think it has deep problems in the sense the prior one did and so should be amenable to laundry listing what we need and I'll move it forward. I think this set of tests should be added/fixed:

A test where the line is broken in the middle and/or first column (line breaking is really the sticky bit)
Fixing the test where the 100 column limit doesn't format correctly
Adding a test with several arrays to format and varying the other alignment options to make sure that doesn't confuse anything

I guess a final question I have would be, if we get this list sorted who can/would be capable of committing this change to the repo?

In D101868#2775550, @feg208 wrote:

This reworks substantially this commit. I recognize there are lacking/broken
tests but I just would like to ensure that the general direction doesn't
seem likely to end in tears

For me it's good, but please wait for @MyDeveloperDay .

Looks good, but please add a test with the else IF and just else case.

In D102706#2766871, @vlovich wrote:

In D102706#2766680, @HazardyKnusperkeks wrote:

Maybe a bit more test cases with smaller lambdas? Or without the outer parenthesis?

I'm not sure I understand this comment. Which test case are you referring to by "or without the outer parenthesis"?

Maybe a bit more test cases with smaller lambdas? Or without the outer parenthesis?

Apart from that comment I don't have any objections. :)

I second that ColumnLimit breaking test case. :)

Something which just came to my mind. Since you wrote your own LineFormatter, you have to add test cases for all kinds of indention and wrapping. It seems that the string wrapping does not happen, how about comments? Are classes, namespaces, functions, etc. correctly indented with various configurations?

Looks good, but please wait for others to state their opinion.

I have not a great knowledge about python, but it looks okay.

I really like alignment! :)

Jep, that would also confuse myself.

Looks good, and I'm always in favor of having a Leave Option. :)

LGTM but please wait for @MyDeveloperDay opinion.

Looks okay for me, but please fix the formatting notes.

Looks good, but please fix the clang-format notes.

In D100727#2698422, @curdeius wrote:

More I look at the current state of this option, more I think it's misleading.
I would expect that the if after else is also controlled by this option. And, the last else as well (adding yet another option for that seems to be a bit an overkill)

@klimek, @MyDeveloperDay, I'd love to hear your input as well on the best course of action, as I've seen that you've discussed this topic a bit in https://reviews.llvm.org/D59087.

The current status quo looks like:

/// Different styles for handling short if statements.
enum ShortIfStyle : unsigned char {
  /// Never put short ifs on the same line.
  /// \code
  ///   if (a)
  ///     return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Never,
  /// Put short ifs on the same line only if there is no else statement.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_WithoutElse,
  /// Always put short ifs on the same line.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b) return;
  ///   else
  ///     return;
  ///
  ///   if (c) return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Always,
};

I.e. WithoutElse does not depend on the else statement being compound or not. I think I'll push a NFC commit to fix documentation and add tests for this.

Anyway, what I'm inclined to do is to have these options:

• Never (same as now)
• WithoutElse (same as now, concerns only a simple if (c) f();)
• OnlyFirstIf (renamed from Always, and Always kept for backward compatibility, it would behave as currently, so only the first if in the sequence of if else if else if else is concerned)
• AllIfsAndElse (would do what I want to achieve in this patch, so format like this:

if (c) f();
else if (c) f();
else if (c) f();
else if();

Naming is hard. All suggestions are welcome :).

In D100727#2697815, @curdeius wrote:

Oh, you're right. Should we go for a separate option then? AllowShortElseStatementsOnASingleLine? What would control else if statements then?

LGTM

In D100727#2697490, @curdeius wrote:

In D100727#2697419, @HazardyKnusperkeks wrote:

How is

if (a) return;
else
  return;

formatted with the different options?

Do you have something specific in mind?

How is

if (a) return;
else
  return;

formatted with the different options?

In D92168#2693972, @anastasiia_lukianenko wrote:

In D92168#2690211, @HazardyKnusperkeks wrote:

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

The patch was originally made as a fix for unexpected behavior. I'm not sure that someone would use a variant where the macro is not recognized as a case block... That's why I don't think the new flag is really needed.

Trigger build.

I don't know if you did elsewhere, but you have to give a name and email for the commit, so that someone can push it for you.

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

Look good in general, only the few comments.