In D150403#4539874, @owenpan wrote:

This seems to cause a regression. See https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?

Addresses latest review comments.

In D151761#4524653, @owenpan wrote:

FWIW, I think we can use a shorter name AlignConsecutiveCaseStatements instead of AlignConsecutiveShortCaseStatements.

In D151761#4483050, @HazardyKnusperkeks wrote:

Thanks for the patience, I'm really looking forward to use this.

But please wait for other opinions.

Addressed outstanding review comments.

This iteration switches away from using AlignConsecutiveStyle and instead uses a new ShortCaseStatementsAlignmentStyle.

I re-wrote the alignment to stop using AlignTokens so that I can now handle all the edge cases that came up. Specifically:

• Allowing empty case labels (implicit fall through) to not break the alignment, but only if they are sandwiched by short case statements.
• Don't align the colon of a non-short case label that follows short case labels when using 'AlignCaseColons=true'.
• Empty case labels will also now push out the alignment of the statements when using AlignCaseColons=false.

Well, I guess I didn't put quite enough thought into the AlignCaseColons option. While this solves the empty case label problem, it will also match in non-short case label scenarios such as the following, which doesn't seem desirable:

Ok, I added the ability to align the case label colons. In your original message you mentioned "I'd like to align the colon (and thus the statement behind that)" which implies actually adding the whitespace after the 'case' token itself. Not sure if that would still be your preference in an ideal world, or if I just misinterpreted your request. Aligning the colons themselves is very straightforward.

In D151761#4404179, @HazardyKnusperkeks wrote:

In D151761#4403828, @galenelias wrote:

In D151761#4400693, @HazardyKnusperkeks wrote:

I'd say: align it.

If it was straightforward, I would definitely agree to just align across empty case labels, but unfortunately there is no way to do that while using the generic AlignTokens (since the line with just a case has no token which matches the pattern, but needs to not break the alignment streak). I guess the way to do it generically would be to add some sort of ExcludeLine lambda to allow filtering out lines. Given that I don't think this is a common pattern with these 'Short Case Labels', and it's fairly easy to work around with [[fallthrough]]; my plan is just to leave this as is (and add a test to make the behavior explicit).

Leaving it open (and documenting that) I could get behind, but making it explicit as desired behavior will not get my approval. On the other hand I will not stand in the way, if the others approve.

In D151761#4400693, @HazardyKnusperkeks wrote:

In D151761#4400056, @galenelias wrote:

In D151761#4394758, @MyDeveloperDay wrote:

did you consider a case where the case falls through? (i.e. no return)

"case log::info :    return \"info\";\n"
"case log::warning :\n"
"default :           return \"default\";\n"

That's a great point. I didn't really consider this, and currently this particular case won't align the case statements if they have an empty case block, however if you had some tokens (e.g. // fallthrough) it would. It's not immediately clear to me what the expectation would be. I guess to align as if there was an invisible trailing token, but it's a bit awkward if the cases missing a body are the 'long' cases that push out the alignment. Also, I don't think it's possible to use AlignTokens and get this behavior, as there is no token on those lines to align, so it's not straightforward to handle. I guess I'll be curious to see if there is feedback or cases where this behavior is desired, and if so, I can look into adding that functionality later. Since right now it would involve a completely custom AlignTokens clone, my preference would be to just leave this as not supported.

I'd say: align it.

In D151761#4394758, @MyDeveloperDay wrote:

did you consider a case where the case falls through? (i.e. no return)

"case log::info :    return \"info\";\n"
"case log::warning :\n"
"default :           return \"default\";\n"

Fixup up review comments.

In D151761#4385163, @HazardyKnusperkeks wrote:

When I read the title I thought "Yay, some one is doing what I want and don't find the time.", but (for me) sadly you're not.
I'd like to align the colon (and thus the statement behind that). Do you think you can add that option too?

Thanks for the additional review comments. Hopefully everything if fixed.

In D150403#4347323, @HazardyKnusperkeks wrote:

We'll wait a bit, if someone might have a comment. And (at least I) need name and email for the commit.

Thanks @HazardyKnusperkeks! I don't have commit access, so will need someone to land this for me.

Thanks for the review and feedback. Sorry about the unfortunate timing between @sstwcw and my fix - we submitted our fixes less than 24 hours apart. I didn't think there would be another simultaneous fix for this 5 year old bug, so didn't even think to link to the review in the GitHub issue. I will definitely do this for any future contributions.

Addressed remaining review feedback.

@HazardyKnusperkeks, thanks for the feedback. I added the TokenAnnotatorTests from @sstwcw's review. I also updated the design to use a single stack instead of the two parallel stacks.

Coincidentally I also sent out a review to fix this issue yesterday, but went with a different approach of trying to scope the ProbablyBracedList logic by just looking at the lbrace previous token.

Looks like @sstwcw also submitted a fix for the same issue, with a bit of a different approach: https://reviews.llvm.org/D150452