[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Galen Elias via Phabricator via cfe-commits]

galenelias added a comment.

In D151761#4400693 <https://reviews.llvm.org/D151761#4400693>, @HazardyKnusperkeks wrote:

> In D151761#4400056 <https://reviews.llvm.org/D151761#4400056>, @galenelias wrote:
>
>> In D151761#4394758 <https://reviews.llvm.org/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.

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

In D151761#4402568 <https://reviews.llvm.org/D151761#4402568>, @MyDeveloperDay wrote:

> In D151761#4400056 <https://reviews.llvm.org/D151761#4400056>, @galenelias wrote:
>
>> In D151761#4394758 <https://reviews.llvm.org/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.
>
> Can you add a unit test with a // fallthough comment, and a /* FALLTHROUGH */ and [[fallthrough]] so we know whats going to happen in those cases at a minimum.

Done.  This actually caught an issue I wasn't aware of where if you use `//fallthrough` and the comment needs to be reflowed, you get two `Change` entries pointing to the same comment token, which throws off the algorithm, and results in broken alignment.  Attempted to fix it up to ignore the inner reflow change and it seems to work in my basic validation.


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

https://reviews.llvm.org/D151761