[PATCH] D151761: clang-format: Add AlignConsecutiveShortCaseStatements

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


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

And with C++20 you don't need this anymore (although I don't know when llvm will switch to c++20).


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:108
   alignConsecutiveAssignments();
+  alignConsecutiveShortCaseStatements();
   alignChainedConditionals();
----------------
galenelias wrote:
> HazardyKnusperkeks wrote:
> > Is this the right position?
> > Could you add a test with aligning assignments and case statements?
> > 
> > ```
> > case XX: xx = 2; break;
> > case X: x = 1; break;
> > ```
> > It should end up as
> > ```
> > case XX: xx = 2; break;
> > case X:  x  = 1; break;
> > ``` 
> Great point, I hadn't actually really fully considered the ordering dependency here.  I agree that aligning short case statements before assignments (and probably declarations?) seems more correct.  However, these other alignConsecutive* methods don't actually align across scopes, so won't align declarations/assignments within the body of a case statement.  I verified that these don't align today using AlignConsecutiveAssignments.  I will however move this up to try to make it as logically correct as possible.
Too bad, that would really be nice. :)
But certainly out of scope for this change.


================
Comment at: clang/unittests/Format/ConfigParseTest.cpp:321
   CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveDeclarations);
+  CHECK_ALIGN_CONSECUTIVE(AlignConsecutiveShortCaseStatements);
 
----------------
You now have to write your own checks for the parsing. (It's just copy & paste.)


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

https://reviews.llvm.org/D151761