[PATCH] D158795: Fix AlignArrayOfStructures + Cpp11BracedListStyle=false

[Galen Elias via Phabricator via cfe-commits]

galenelias marked an inline comment as done.
galenelias added inline comments.


================
Comment at: clang/lib/Format/WhitespaceManager.cpp:1247
         if (Previous && Previous->isNot(TT_LineComment)) {
-          Changes[Next->Index].Spaces = 0;
+          Changes[Next->Index].Spaces = BracePadding;
           Changes[Next->Index].NewlinesBefore = 0;
----------------
owenpan wrote:
> owenpan wrote:
> > Can we assert that `Spaces == 0`? If not, we should add a test case.
> We can't assert that, but setting `Spaces` here seems superfluous as it's set correctly below anyways?
I admit I'm not super confident on my understanding of this code, but this setting of Spaces is not redundant with any below settings.  If we set it to '3' for instance, that won't get overwritten later (because the other sets are all conditional, and don't hit for the `}` token).

So, I think this is still required (minus the issue of the existing 'Spaces' calculation from previous formatting pass seemingly already setting Spaces to the correct value).


================
Comment at: clang/unittests/Format/FormatTest.cpp:20888
+               "  {  7,     5,    \"!!\" }\n"
+               "};\n",
+               Style);
----------------
owenpan wrote:
> 
This is consistent with basically every single adjacent test in this function.  While I agree that this is unnecessary, in general I error on the side of consistency with the surrounding tests.  I'll defer to the maintainers, just wanted to make sure this is actually the preferred change given the numerous adjacent tests with this form.


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

https://reviews.llvm.org/D158795