[clang-format] Recognize Verilog non-blocking assignment

[HazardyKnusperkeks]

clang/lib/Format/WhitespaceManager.cpp

839–847

Do you need the extra case, or could you just activate AlignCompound?

If you do, can you please add parens, the precedence of ? and : in combination with || is at least for me not 100% clear.

[sstwcw]

• add parentheses

[sstwcw]

clang/lib/Format/WhitespaceManager.cpp

839–847

If I get you right, you are suggesting that AlignCompound: false is there only for backward compatibility, and we can make it default to true since we don't have to maintain backward compatibility for new things. I prefer consistency.

[HazardyKnusperkeks]

Looks ok to me. Please wait for some other opinion.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

1650

1968–1969

Can you move them into the if statement above?

[sstwcw]

• add parentheses
• rename field

clang/lib/Format/TokenAnnotator.cpp

struct Context {
     bool ColonIsDictLiteral = false;
     bool ColonIsObjCMethodExpr = false;
     FormatToken *FirstObjCSelectorName = nullptr;
     FormatToken *FirstStartOfName = nullptr;
     bool CanBeExpression = true;
     bool CaretFound = false;
     bool InCpp11AttributeSpecifier = false;
     bool InCSharpAttributeSpecifier = false;
+    bool VerilogAssignmentFound = false;
     enum {
       Unknown,
       // Like the part after `:` in a constructor.
       //   Context(...) : IsExpression(IsExpression)
       CtorInitializer,
       // Like in the parentheses in a foreach.
       ForEachMacro,
       // Like the inheritance list in a class declaration.
if (Current.is(Keywords.kw_instanceof)) {
         Current.setType(TT_JsTypeOptionalQuestion);
       } else {
         Current.setType(TT_ConditionalExpr);
       }
     } else if (Current.isBinaryOperator() &&
                (!Current.Previous || Current.Previous->isNot(tok::l_square)) &&
                (!Current.is(tok::greater) &&
                 Style.Language != FormatStyle::LK_TextProto)) {
+      if (Style.isVerilog()) {
+        if (Current.is(tok::lessequal) && Contexts.size() == 1 &&
+            !Contexts.back().VerilogAssignmentFound) {
+          // In Verilog `<=` is assignment if in its own statement. It is a
+          // statement instead of an expression, that is it can not be chained.
+          Current.ForcedPrecedence = prec::Assignment;
+          Current.setFinalizedType(TT_BinaryOperator);
+        }
+        if (Current.getPrecedence() == prec::Assignment)
+          Contexts.back().VerilogAssignmentFound = true;
+      }
       Current.setType(TT_BinaryOperator);
     } else if (Current.is(tok::comment)) {
       if (Current.TokenText.startswith("/*")) {
         if (Current.TokenText.endswith("*/")) {
           Current.setType(TT_BlockComment);
         } else {
           // The lexer has for some reason determined a comment here. But we
           // cannot really handle it, if it isn't properly terminated.
           Current.Tok.setKind(tok::unknown);
         }
       } else {
         Current.setType(TT_LineComment);
       }
     } else if (Current.is(tok::l_paren)) {
       if (lParenStartsCppCast(Current))
         Current.setType(TT_CppCastLParen);
     } else if (Current.is(tok::r_paren)) {
       if (rParenEndsCast(Current))
         Current.setType(TT_CastRParen);

clang/lib/Format/WhitespaceManager.cpp

AlignTokens(
         if (&C != &Changes.back() && (&C + 1)->NewlinesBefore > 0)
           return false;
 
         // Do not align operator= overloads.
         FormatToken *Previous = C.Tok->getPreviousNonComment();
         if (Previous && Previous->is(tok::kw_operator))
           return false;
 
         return Style.AlignConsecutiveAssignments.AlignCompound
                    ? C.Tok->getPrecedence() == prec::Assignment
-                   : C.Tok->is(tok::equal);
+                   : (C.Tok->is(tok::equal) ||
+                      // In Verilog the '<=' is not a compound assignment, thus
+                      // it is aligned even when the AlignCompound option is not
+                      // set.
+                      (Style.isVerilog() && C.Tok->is(tok::lessequal) &&
+                       C.Tok->getPrecedence() == prec::Assignment));
       },
       Changes, /*StartAt=*/0, Style.AlignConsecutiveAssignments,
       /*RightJustify=*/true);
 }
 
 void WhitespaceManager::alignConsecutiveBitFields() {
   if (!Style.AlignConsecutiveBitFields.Enabled)
     return;
 

clang/unittests/Format/FormatTestVerilog.cpp

static void verifyFormat(
       llvm::StringRef Code,
       const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Verilog)) {
     EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(),
               format(test::messUp(Code, /*HandleHash=*/false), Style));
   }
 };
 
+TEST_F(FormatTestVerilog, Align) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.AlignConsecutiveAssignments.Enabled = true;
+  verifyFormat("x            <= x;\n"
+               "sfdbddfbdfbb <= x;\n"
+               "x             = x;",
+               Style);
+  verifyFormat("x            = x;\n"
+               "sfdbddfbdfbb = x;\n"
+               "x            = x;",
+               Style);
+  // Compound assignments are not aligned by default. '<=' is not a compound
+  // assignment.
+  verifyFormat("x            <= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x += x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x <<= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x <<<= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x >>= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x >>>= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  Style.AlignConsecutiveAssignments.AlignCompound = true;
+  verifyFormat("x            <= x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x            += x;\n"
+               "sfdbddfbdfbb <= x;",
+               Style);
+  verifyFormat("x            <<= x;\n"
+               "sfdbddfbdfbb  <= x;",
+               Style);
+  verifyFormat("x            <<<= x;\n"
+               "sfdbddfbdfbb   <= x;",
+               Style);
+  verifyFormat("x            >>= x;\n"
+               "sfdbddfbdfbb  <= x;",
+               Style);
+  verifyFormat("x            >>>= x;\n"
+               "sfdbddfbdfbb   <= x;",
+               Style);
+}
+
 TEST_F(FormatTestVerilog, BasedLiteral) {
   verifyFormat("x = '0;");
   verifyFormat("x = '1;");
   verifyFormat("x = 'X;");
   verifyFormat("x = 'x;");
   verifyFormat("x = 'Z;");
   verifyFormat("x = 'z;");
   verifyFormat("x = 659;");

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsVerilogOperators) {
   Tokens = Annotate("case (x)\n"
                     "  x ? x : x:\n"
                     "    x;\n"
                     "endcase\n");
   ASSERT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[5], tok::question, TT_ConditionalExpr);
   EXPECT_TOKEN(Tokens[7], tok::colon, TT_ConditionalExpr);
   EXPECT_TOKEN(Tokens[9], tok::colon, TT_GotoLabelColon);
+  // Non-blocking assignments.
+  Tokens = Annotate("a <= b;");
+  ASSERT_EQ(Tokens.size(), 5u);
+  EXPECT_TOKEN(Tokens[1], tok::lessequal, TT_BinaryOperator);
+  EXPECT_TOKEN_PRECEDENCE(Tokens[1], prec::Assignment);
+  Tokens = Annotate("if (a <= b) break;");
+  ASSERT_EQ(Tokens.size(), 9u);
+  EXPECT_TOKEN(Tokens[3], tok::lessequal, TT_BinaryOperator);
+  EXPECT_TOKEN_PRECEDENCE(Tokens[3], prec::Relational);
+  Tokens = Annotate("a <= b <= a;");
+  ASSERT_EQ(Tokens.size(), 7u);
+  EXPECT_TOKEN(Tokens[1], tok::lessequal, TT_BinaryOperator);
+  EXPECT_TOKEN_PRECEDENCE(Tokens[1], prec::Assignment);
+  EXPECT_TOKEN(Tokens[3], tok::lessequal, TT_BinaryOperator);
+  EXPECT_TOKEN_PRECEDENCE(Tokens[3], prec::Relational);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandConstructors) {
   auto Tokens = annotate("Class::Class() : BaseClass(), Member() {}");
 
   // The TT_Unknown is clearly not binding for the future, please adapt if those
   // tokens get annotated.
   ASSERT_EQ(Tokens.size(), 16u) << Tokens;