[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

This revision was automatically updated to reflect the committed changes. Closed by commit rG4ba00844174d: [clang-format] Add AlignConsecutiveShortCaseStatements (authored by owenpan). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. All of those variants are fine by me, but I'd stick with the current version, because it is linked to `AllowShortCaseLabelsOnASingleLine`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

owenpan accepted this revision. owenpan added a comment. In D151761#4528389 , @galenelias wrote: > In D151761#4524653 , @owenpan wrote: > >> FWIW, I think we can use a shorter name

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 543558. galenelias added a comment. Addresses latest review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/ https://reviews.llvm.org/D151761 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked 6 inline comments as done. galenelias added a comment. In D151761#4524653 , @owenpan wrote: > FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` > instead of `AlignConsecutiveShortCaseStatements`. My only

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

owenpan added a comment. FWIW, I think we can use a shorter name `AlignConsecutiveCaseStatements` instead of `AlignConsecutiveShortCaseStatements`. Comment at: clang/lib/Format/WhitespaceManager.cpp:854 + return C.Tok->is(TT_CaseLabelColon); +} else { + //

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias added a comment. In D151761#4483050 , @HazardyKnusperkeks wrote: > Thanks for the patience, I'm really looking forward to use this. > > But please wait for other opinions. Thanks so much @HazardyKnusperkeks for all the feedback and help.

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. This revision is now accepted and ready to land. Thanks for the patience, I'm really looking forward to use this. But please wait for other opinions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 538288. galenelias marked 2 inline comments as done. galenelias added a comment. Addressed outstanding review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/ https://reviews.llvm.org/D151761 Files:

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked 6 inline comments as done. galenelias added inline comments. Comment at: clang/include/clang/Format/Format.h:380 +} +bool operator!=(const ShortCaseStatementsAlignmentStyle ) const { + return !(*this == R); HazardyKnusperkeks

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added inline comments. Comment at: clang/include/clang/Format/Format.h:380 +} +bool operator!=(const ShortCaseStatementsAlignmentStyle ) const { + return !(*this == R); I'd drop that. We don't have it for any other struct. And

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 538228. galenelias marked an inline comment as done. galenelias edited the summary of this revision. galenelias added a comment. This iteration switches away from using `AlignConsecutiveStyle` and instead uses a new `ShortCaseStatementsAlignmentStyle`.

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked an inline comment as done. galenelias added inline comments. Comment at: clang/include/clang/Format/Format.h:327 + /// \version 17 + AlignConsecutiveStyle AlignConsecutiveShortCaseStatements; HazardyKnusperkeks wrote: > Since you are not

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added a comment. In D151761#4474136 , @galenelias wrote: > 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 537383. galenelias edited the summary of this revision. galenelias added a comment. 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

owenpan added a comment. In D151761#4418809 , @HazardyKnusperkeks wrote: > 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. I think we should

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added a comment. In 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

owenpan added a comment. In D151761#4410998 , @HazardyKnusperkeks wrote: > In D151761#4410158 , @galenelias > wrote: > >> Yah, I think leaving it open would be my preference at this point. Not sure >> how to

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias added a comment. 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 530707. galenelias edited the summary of this revision. galenelias added a comment. 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added a comment. After I came across some of my code, where I'd really like a feature like this, I'm back at wishing to align the colons. Same as in a bit field. And when you would do that I think there would be no open issue with empty statements, right? So for my code I'd

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added a comment. In D151761#4410158 , @galenelias wrote: > Yah, I think leaving it open would be my preference at this point. Not sure > how to properly document that though? Just be explicit in the tests? > Mention it in

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias added a comment. In D151761#4404179 , @HazardyKnusperkeks wrote: > In D151761#4403828 , @galenelias > wrote: > >> In D151761#4400693 , >>

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added a comment. In D151761#4403828 , @galenelias wrote: > In D151761#4400693 , > @HazardyKnusperkeks wrote: > >> I'd say: align it. > > If it was straightforward, I would definitely agree to

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 529365. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/ https://reviews.llvm.org/D151761 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias added a comment. In D151761#4400693 , @HazardyKnusperkeks wrote: > In D151761#4400056 , @galenelias > wrote: > >> In D151761#4394758 , >> @MyDeveloperDay

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

MyDeveloperDay added a comment. 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks requested changes to this revision. HazardyKnusperkeks added a comment. This revision now requires changes to proceed. In D151761#4400056 , @galenelias wrote: > In D151761#4394758 , >

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked an inline comment as done. galenelias added a comment. 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

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked 3 inline comments as done. galenelias added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:798 + +case log::info: return "info:"; +case log::warning: return "warning:"; MyDeveloperDay wrote: > Do you think the

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

MyDeveloperDay added a comment. 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" CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:798 + +case log::info: return "info:"; +case log::warning: return "warning:"; Do you think the documentation should give examples of the the other cases?

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

MyDeveloperDay added a comment. Apart from the documentation this looks fine.. thats probably outside the scope of this change CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/ https://reviews.llvm.org/D151761 ___ cfe-commits

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

MyDeveloperDay added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:790 + +**AlignConsecutiveShortCaseLabels** (``AlignConsecutiveStyle``) :versionbadge:`clang-format 17` :ref:`¶ ` + Style of aligning consecutive short case labels.

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. This revision is now accepted and ready to land. Please wait for some other approval (or comments) for a few days. Comment at: clang/unittests/Format/FormatTest.cpp:19218 + // Verify comments and

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias marked 3 inline comments as done. galenelias added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:19218 + // Verify comments and empty lines break the alignment + verifyFormat("switch (level) {\n" + "case log::info:return

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 528118. galenelias added a comment. Fixup up review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151761/new/ https://reviews.llvm.org/D151761 Files: clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

HazardyKnusperkeks added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:19217 + + // Verify comments and empty lines break the alignment + verifyFormat("switch (level) {\n" Comment at:

[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

galenelias updated this revision to Diff 528072. galenelias marked 3 inline comments as done. galenelias retitled this revision from "clang-format: Add AlignConsecutiveShortCaseLabels" to "clang-format: Add AlignConsecutiveShortCaseStatements". galenelias edited the summary of this revision.