[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Owen Pan via Phabricator via cfe-commits]

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 {
+      // Ignore 'IsInsideToken' to allow matching trailing comments which
----------------
No `else` after `return`.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:859-860
+      // reflow early due to detecting multiple aligning tokens per line.
+      return (!C.IsInsideToken && C.Tok->Previous &&
+              C.Tok->Previous->is(TT_CaseLabelColon));
+    }
----------------
Please remove the redundant parentheses.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:909
+
+    if (!Changes[I].Tok->is(tok::comment))
+      LineIsComment = false;
----------------



================
Comment at: clang/lib/Format/WhitespaceManager.cpp:913
+    if (Changes[I].Tok->is(TT_CaseLabelColon)) {
+      LineIsEmptyCase = Changes[I].Tok->Next == nullptr ||
+                        Changes[I].Tok->Next->isTrailingComment();
----------------



================
Comment at: clang/unittests/Format/FormatTest.cpp:19284-19293
+  verifyFormat("switch (level) {\n"
+               "case log::info:    return \"info\";\n"
+               "case log::warning: return \"warning\";\n"
+               "// comment\n"
+               "case log::critical: return \"critical\";\n"
+               "default:            return \"default\";\n"
+               "\n"
----------------



================
Comment at: clang/unittests/Format/FormatTest.cpp:19218
+  // Verify comments and empty lines break the alignment
+  verifyFormat("switch (level) {\n"
+               "case log::info:    return \"info\";\n"
----------------
HazardyKnusperkeks wrote:
> galenelias wrote:
> > HazardyKnusperkeks wrote:
> > > If both strings are the same, you only need to supply it once.
> > > 
> > > Or are they different and I can't see it?
> > They are the same, but I can't use the single string version, as that one calls test::messUp on the string which will strip blank lines, which then invalidates the test case.  It seems pretty much all tests which are trying to validate behavior around empty spaces has to use the two string version.
> Check.
> They are the same, but I can't use the single string version, as that one calls test::messUp on the string which will strip blank lines, which then invalidates the test case.  It seems pretty much all tests which are trying to validate behavior around empty spaces has to use the two string version.

We have `verifyNoChange` for that now.


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

https://reviews.llvm.org/D151761