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