HazardyKnusperkeks added a comment.
In D151761#4416224 <https://reviews.llvm.org/D151761#4416224>, @galenelias
wrote:
> 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:
>
> switch (level) {
> case log::info :
> case log::error :
> return true;
> case log::none :
> case log::warning :
> case log::critical :
> return false;
> }
>
> In order to solve this (and the other) issues, I think the only solution is
> to roll a custom alignment method instead of using `AlignTokens`, so that the
> alignment can be more correctly based on the detection of short case
> statements.
>
> This is going to take considerably more time and code, so not sure when I'll
> be able to work on it.
For me this would actually be desired. But from the name `ShortCaseStatements`
I can see that it may be not what we want. This could be //fixed// with just
renaming to `ConsecutiveCaseStatements`. :)
In D151761#4415755 <https://reviews.llvm.org/D151761#4415755>, @galenelias
wrote:
> 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.
>
> I opted to make this an option on `AlignConsecutiveStyle`, as that is
> consistent with how we customize some of the other AlignConsecutive* options,
> and it seemed awkward to add a floating top level boolean config option which
> applied to just this scenario - although it has the similar downside that it
> muddies the AlignConsecutiveStyle options for the other use cases.
I have no problem with that, but the full disclosure is: @MyDeveloperDay isn't
happy with e.g. `PadOperators` and thus will most likely not like this.
I think we need some input what should be able to be aligned - the statement
vs. the colon - and only //short// case statements vs all case statements.
================
Comment at: clang/include/clang/Format/Format.h:257
+ /// \endcode
+ bool AlignCaseColons;
bool operator==(const AlignConsecutiveStyle &R) const {
----------------
I'd also sort this alphabetically.
================
Comment at: clang/lib/Format/WhitespaceManager.cpp:881
+ return C.Tok->is(TT_CaseLabelColon);
+ } else {
+ // Ignore 'InsideToken' to allow matching trailing comments which
----------------
else after return can be dropped.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151761/new/
https://reviews.llvm.org/D151761
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
