[PATCH] D154755: [clang-format] Fix formatting of if statements with BlockIndent

[Gedare Bloom via Phabricator via cfe-commits]

gedare added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:25504
+
+#if 0
   verifyFormat("if (quitelongarg !=\n"
----------------
MyDeveloperDay wrote:
> we don't do this.
OK, I'll move that other bug up my priority list, as the behavior of this test case involves both. That bug prevents breaking the line after the operand, I think there's some off-by-1 error somewhere in calculating the columns for blockindent breaks. Otherwise, this patch reveals that bug as a regression in this test case.


================
Comment at: clang/unittests/Format/FormatTest.cpp:25505-25510
   verifyFormat("if (quitelongarg !=\n"
                "    (alsolongarg - 1)) { // ABC is a very longgggggggggggg "
                "comment\n"
                "  return;\n"
                "}",
                Style);
----------------
MyDeveloperDay wrote:
> you can't remove a test and just call it good... the original author put this test in for a reason I assume?
understood, although it seems that the original author may not have considered `if` statements as legitimately to be used in BlockIndented formats, since this test case does not block indent. So, the test case will have to change anyway, if the `if` should be blockindented.


================
Comment at: clang/unittests/Format/FormatTest.cpp:25514
+  verifyFormat("void foo() {\n"
+               "  if (quitelongname < alsolongname ||\n"
+               "      anotherevenlongername <=\n"
----------------
MyDeveloperDay wrote:
> isn't the breaking of `if (` and `} else if (\n` inconsistent?
The breaking is dependent on what ends up exceeding the ColumnLimit. since ` if (` is shorter than ` } else if (` by about 5 characters, you can get different breaks. As far as I can tell, the behavior of `AlwaysBreak` and `BlockIndent` will only break after the opening parens if the first "argument" doesn't fit. From what I understand, the penalties and desirable breaks come into play then, based on breaking around commas or boolean operators.  With this patch, the behavior here is now consistent for both `AlwaysBreak` and `BlockIndent`. 

This behavior is undocumented and I do not know the intention. I would think it is better for the two options to behave consistently, but I'm also willing to treat them differently for backward compatibility while fixing this bug.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154755