r185530 - Don't insert confusing line breaks in comparisons.

[Daniel Jasper]

Author: djasper
Date: Wed Jul  3 05:34:47 2013
New Revision: 185530

URL: http://llvm.org/viewvc/llvm-project?rev=185530&view=rev
Log:
Don't insert confusing line breaks in comparisons.

In general, clang-format breaks after an operator if the LHS spans
multiple lines. Otherwise, this can lead to confusing effects and
effectively hide the operator precendence, e.g. in

if (aaaaaaaaaaaaaa ==
        bbbbbbbbbbbbbb && c) { ...

This patch removes this rule for comparisons, if the LHS is not a binary
expression itself as many users were wondering why clang-format inserts
an unnecessary linebreak.

Before:
if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) >
    5) { ...

After:
if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) > 5) { ...

In the long run, we might:
- Want to do this for other binary expressions as well.
- Do this only if the RHS is short or even only if it is a literal.

Modified:
    cfe/trunk/lib/Format/Format.cpp
    cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/Format.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=185530&r1=185529&r2=185530&view=diff
==============================================================================
--- cfe/trunk/lib/Format/Format.cpp (original)
+++ cfe/trunk/lib/Format/Format.cpp Wed Jul  3 05:34:47 2013
@@ -1066,9 +1066,23 @@ private:
       return true;
 
     // If we need to break somewhere inside the LHS of a binary expression, we
-    // should also break after the operator.
+    // should also break after the operator. Otherwise, the formatting would
+    // hide the operator precedence, e.g. in:
+    //   if (aaaaaaaaaaaaaa ==
+    //           bbbbbbbbbbbbbb && c) {..
+    // For comparisons, we only apply this rule, if the LHS is a binary
+    // expression itself as otherwise, the line breaks seem superfluous.
+    // We need special cases for ">>" which we have split into two ">" while
+    // lexing in order to make template parsing easier.
+    bool IsComparison = (Previous.getPrecedence() == prec::Relational ||
+                         Previous.getPrecedence() == prec::Equality) &&
+                        Previous.Previous &&
+                        Previous.Previous->Type != TT_BinaryOperator; // For >>.
+    bool LHSIsBinaryExpr =
+        Previous.Previous && Previous.Previous->FakeRParens > 0;
     if (Previous.Type == TT_BinaryOperator &&
-        Current.Type != TT_BinaryOperator && // Special case for ">>".
+        (!IsComparison || LHSIsBinaryExpr) &&
+        Current.Type != TT_BinaryOperator && // For >>.
         !Current.isTrailingComment() &&
         !Previous.isOneOf(tok::lessless, tok::question) &&
         Previous.getPrecedence() != prec::Assignment &&

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=185530&r1=185529&r2=185530&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Jul  3 05:34:47 2013
@@ -2156,6 +2156,38 @@ TEST_F(FormatTest, LineBreakingInBinaryE
                "     bbbbbbbbbbbbbbbbbb) && // aaaaaaaaaaaaaaaa\n"
                "    cccccc) {\n}");
 
+  // If the LHS of a comparison is not a binary expression itself, the
+  // additional linebreak confuses many people.
+  verifyFormat(
+      "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+      "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) > 5) {\n"
+      "}");
+  verifyFormat(
+      "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+      "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) == 5) {\n"
+      "}");
+  // Even explicit parentheses stress the precedence enough to make the
+  // additional break unnecessary.
+  verifyFormat(
+      "if ((aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n"
+      "     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) == 5) {\n"
+      "}");
+  // This cases is borderline, but with the indentation it is still readable.
+  verifyFormat(
+      "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+      "        aaaaaaaaaaaaaaa) > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n"
+      "                               aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
+      "}",
+      getLLVMStyleWithColumns(75));
+
+  // If the LHS is a binary expression, we should still use the additional break
+  // as otherwise the formatting hides the operator precedence.
+  verifyFormat(
+      "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +\n"
+      "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ==\n"
+      "    5) {\n"
+      "}");
+
   FormatStyle OnePerLine = getLLVMStyle();
   OnePerLine.BinPackParameters = false;
   verifyFormat(