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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
