[clang-format] Put ports on separate lines in Verilog module headers

[sstwcw]

Summary

New:

module mh1
    (input var int in1,
     input var in2, in3,
     output tagged_st out);
endmodule

Old:

module mh1
    (input var int in1, input var in2, in3, output tagged_st out);
endmodule

getNextNonComment was modified to return a non-const pointer because
we needed to use it that way in verilogGroupDecl.

The comment on line 2620 was a typo. We corrected it while modifying
the function.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2842

are we covering these cases in the unit tests

2852

are we covering these cases in the unit tests

4375

Is this an unrelated change? if not can you add an test

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

2842

I added a test on line 432.

2852

I added a test on line 374.

4375

I added a test on line 255.

[sstwcw]

• add tests

[MyDeveloperDay]

Thank you for adding the tests, as I don't know Verilog then I can't really comment on the correctness, as you are mostly in your own scoped verilog functions, I'm fine with you improving the Verilog support, I don't personally see anything wrong with the touch points with other languages. I'm not sure if others have comments or if they have expertise in this area.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2665

And other places as well.

2808

And other places too.

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

2665

@HazardyKnusperkeks Do you agree? I remember you once said you preferred the nullptr style.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2665

See D144355. Not using nullptr in conditionals was one of the first LLVM styles I had to get used to when I started contributing.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2665

Apart from Owens comment about the style guide. My personal preference is without nullptr, but I don't know if I maybe said otherwise sometime.

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   /* like in begin : block */                                                  \
   TYPE(VerilogBlockLabelColon)                                                 \
   /* The square bracket for the dimension part of the type name.               \
    * In 'logic [1:0] x[1:0]', only the first '['. This way we can have space   \
    * before the first bracket but not the second. */                           \
   TYPE(VerilogDimensionedTypeName)                                             \
   /* for the base in a number literal, not including the quote */              \
   TYPE(VerilogNumberBase)                                                      \
+  /* like `(strong1, pull0)` */                                                \
+  TYPE(VerilogStrength)                                                        \
   /* Things inside the table in user-defined primitives. */                    \
   TYPE(VerilogTableItem)                                                       \
+  /* those that separate ports of different types */                           \
+  TYPE(VerilogTypeComma)                                                       \
   TYPE(Unknown)
 
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.
 enum TokenType : uint8_t {
 #define TYPE(X) TT_##X,
   LIST_TOKEN_TYPES
 #undef TYPE
bool isVerilogEndOfLabel(const FormatToken &Tok) const {
     const FormatToken *Next = Tok.getNextNonComment();
     // In Verilog the colon in a default label is optional.
     return Tok.is(TT_GotoLabelColon) ||
            (Tok.is(tok::kw_default) &&
             !(Next && Next->isOneOf(tok::colon, tok::semi, kw_clocking, kw_iff,
                                     kw_input, kw_output, kw_sequence)));
   }
 
+  bool isVerilogQualifier(const FormatToken &Tok) const {
+    switch (Tok.Tok.getKind()) {
+    case tok::kw_extern:
+    case tok::kw_signed:
+    case tok::kw_static:
+    case tok::kw_unsigned:
+    case tok::kw_virtual:
+      return true;
+    case tok::identifier:
+      return Tok.isOneOf(
+          kw_let, kw_var, kw_ref, kw_automatic, kw_bins, kw_coverpoint,
+          kw_ignore_bins, kw_illegal_bins, kw_inout, kw_input, kw_interconnect,
+          kw_local, kw_localparam, kw_output, kw_parameter, kw_pure, kw_rand,
+          kw_randc, kw_scalared, kw_specparam, kw_tri, kw_tri0, kw_tri1,
+          kw_triand, kw_trior, kw_trireg, kw_uwire, kw_vectored, kw_wand,
+          kw_wildcard, kw_wire, kw_wor);
+    default:
+      return false;
+    }
+  }
+
 private:
   /// The JavaScript keywords beyond the C++ keyword set.
   std::unordered_set<IdentifierInfo *> JsExtraKeywords;
 
   /// The C# keywords beyond the C++ keyword set
   std::unordered_set<IdentifierInfo *> CSharpExtraKeywords;
 
   /// The Verilog keywords beyond the C++ keyword set.

clang/lib/Format/TokenAnnotator.cpp

void parse(int Precedence = 0) {
     if (Precedence == PrecedenceUnaryOperator) {
       parseUnaryOperator();
       return;
     }
 
     FormatToken *Start = Current;
     FormatToken *LatestOperator = nullptr;
     unsigned OperatorIndex = 0;
+    // The first name of the current type in a port list.
+    FormatToken *VerilogFirstOfType = nullptr;
 
     while (Current) {
+      // In Verilog ports in a module header that don't have a type take the
+      // type of the previous one.  For example,
+      //   module a(output b,
+      //                   c,
+      //            output d);
+      // In this case there need to be fake parentheses around b and c.
+      if (Style.isVerilog() && Precedence == prec::Comma) {
+        VerilogFirstOfType =
+            verilogGroupDecl(VerilogFirstOfType, LatestOperator);
+      }
+
       // Consume operators with higher precedence.
       parse(Precedence + 1);
 
       int CurrentPrecedence = getCurrentPrecedence();
 
       if (Precedence == CurrentPrecedence && Current &&
           Current->is(TT_SelectorName)) {
         if (LatestOperator)
           addFakeParenthesis(Start, prec::Level(Precedence));
         Start = Current;
       }
 
-      // At the end of the line or when an operator with higher precedence is
+      // At the end of the line or when an operator with lower precedence is
       // found, insert fake parenthesis and return.
       if (!Current ||
           (Current->closesScope() &&
            (Current->MatchingParen || Current->is(TT_TemplateString))) ||
           (CurrentPrecedence != -1 && CurrentPrecedence < Precedence) ||
           (CurrentPrecedence == prec::Conditional &&
            Precedence == prec::Assignment && Current->is(tok::colon))) {
         break;
while (Current) {
           LatestOperator = Current;
           Current->OperatorIndex = OperatorIndex;
           ++OperatorIndex;
         }
         next(/*SkipPastLeadingComments=*/Precedence > 0);
       }
     }
 
+    // Group variables of the same type.
+    if (Style.isVerilog() && Precedence == prec::Comma &&
+        VerilogFirstOfType != nullptr) {
+      addFakeParenthesis(VerilogFirstOfType, prec::Comma);
+    }
+
     if (LatestOperator && (Current || Precedence > 0)) {
       // The requires clauses do not neccessarily end in a semicolon or a brace,
       // but just go over to struct/class or a function declaration, we need to
       // intervene so that the fake right paren is inserted correctly.
       auto End =
           (Start->Previous &&
            Start->Previous->isOneOf(TT_RequiresClause,
                                     TT_RequiresClauseInARequiresExpression))
if (Current)
       Current = Current->Next;
     while (Current &&
            (Current->NewlinesBefore == 0 || SkipPastLeadingComments) &&
            Current->isTrailingComment()) {
       Current = Current->Next;
     }
   }
 
+  // Add fake parenthesis around declarations of the same type for example in a
+  // module prototype. Return the first port / variable of the current type.
+  FormatToken *verilogGroupDecl(FormatToken *FirstOfType,
+                                FormatToken *PreviousComma) {
+    if (Current == nullptr)
+      return nullptr;
+
+    FormatToken *Start = Current;
+
+    // Skip attributes.
+    while (Start->startsSequence(tok::l_paren, tok::star)) {
+      if ((Start = Start->MatchingParen) == nullptr ||
+          (Start = Start->getNextNonComment()) == nullptr) {
+        return nullptr;
+      }
+    }
+
+    FormatToken *Tok = Start;
+
+    if (Tok->is(Keywords.kw_assign))
+      Tok = Tok->getNextNonComment();
+
+    // Skip any type qualifiers to find the first identifier. It may be either a
+    // new type name or a variable name. There can be several type qualifiers
+    // preceding a variable name, and we can not tell them apart by looking at
+    // the word alone since a macro can be defined as either a type qualifier or
+    // a variable name. Thus we use the last word before the dimensions instead
+    // of the first word as the candidate for the variable or type name.
+    FormatToken *First = nullptr;
+    while (Tok != nullptr) {
+      FormatToken *Next = Tok->getNextNonComment();
+
+      if (Tok->is(tok::hash)) {
+        // Start of a macro expansion.
+        First = Tok;
+        Tok = Next;
+        if (Tok != nullptr)
+          Tok = Tok->getNextNonComment();
+      } else if (Tok->is(tok::hashhash)) {
+        // Concatenation. Skip.
+        Tok = Next;
+        if (Tok != nullptr)
+          Tok = Tok->getNextNonComment();
+      } else if ((Keywords.isVerilogQualifier(*Tok) ||
+                  Keywords.isVerilogIdentifier(*Tok))) {
+        First = Tok;
+        Tok = Next;
+        // The name may have dots like `interface_foo.modport_foo`.
+        while (Tok != nullptr && Tok->isOneOf(tok::period, tok::coloncolon) &&
+               (Tok = Tok->getNextNonComment())) {
+          if (Keywords.isVerilogIdentifier(*Tok))
+            Tok = Tok->getNextNonComment();
+        }
+      } else if (Next == nullptr) {
+        Tok = nullptr;
+      } else if (Tok->is(tok::l_paren)) {
+        // Make sure the parenthesized list is a drive strength. Otherwise the
+        // statement may be a module instantiation in which case we have already
+        // found the instance name.
+        if (Next->isOneOf(
+                Keywords.kw_highz0, Keywords.kw_highz1, Keywords.kw_large,
+                Keywords.kw_medium, Keywords.kw_pull0, Keywords.kw_pull1,
+                Keywords.kw_small, Keywords.kw_strong0, Keywords.kw_strong1,
+                Keywords.kw_supply0, Keywords.kw_supply1, Keywords.kw_weak0,
+                Keywords.kw_weak1)) {
+          Tok->setType(TT_VerilogStrength);
+          Tok = Tok->MatchingParen;
+          if (Tok != nullptr) {
+            Tok->setType(TT_VerilogStrength);
+            Tok = Tok->getNextNonComment();
+          }
+        } else {
+          break;
+        }
+      } else if (Tok->is(tok::hash)) {
+        if (Next->is(tok::l_paren))
+          Next = Next->MatchingParen;
+        if (Next != nullptr)
+          Tok = Next->getNextNonComment();
+      } else {
+        break;
+      }
+    }
+
+    // Find the second identifier. If it exists it will be the name.
+    FormatToken *Second = nullptr;
+    // Dimensions.
+    while (Tok != nullptr && Tok->is(tok::l_square) &&
+           (Tok = Tok->MatchingParen) != nullptr) {
+      Tok = Tok->getNextNonComment();
+    }
+    if (Tok != nullptr &&
+        (Tok->is(tok::hash) || Keywords.isVerilogIdentifier(*Tok))) {
+      Second = Tok;
+    }
+
+    // If the second identifier doesn't exist and there are qualifiers, the type
+    // is implied.
+    FormatToken *TypedName = nullptr;
+    if (Second != nullptr) {
+      TypedName = Second;
+      if (First != nullptr && First->is(TT_Unknown))
+        First->setType(TT_VerilogDimensionedTypeName);
+    } else if (First != Start) {
+      // If 'First' is null, then this isn't a declaration, 'TypedName' gets set
+      // to null as intended.
+      TypedName = First;
+    }
+
+    if (TypedName != nullptr) {
+      // This is a declaration with a new type.
+      if (TypedName->is(TT_Unknown))
+        TypedName->setType(TT_StartOfName);
+      // Group variables of the previous type.
+      if (FirstOfType != nullptr && PreviousComma != nullptr) {
+        PreviousComma->setType(TT_VerilogTypeComma);
+        addFakeParenthesis(FirstOfType, prec::Comma, PreviousComma->Previous);
+      }
+
+      FirstOfType = TypedName;
+
+      // Don't let higher precedence handle the qualifiers. For example if we
+      // have:
+      //    parameter x = 0
+      // We skip `parameter` here. This way the fake parentheses for the
+      // assignment will be around `x = 0`.
+      while (Current != nullptr && Current != FirstOfType) {
+        if (Current->opensScope()) {
+          next();
+          parse();
+        }
+        next();
+      }
+    }
+
+    return FirstOfType;
+  }
+
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   const AnnotatedLine &Line;
   FormatToken *Current;
 };
 
 } // end anonymous namespace
 
if ((Right.is(Keywords.kw_apostrophe) ||
         (Left.isOneOf(tok::r_square, tok::r_paren, tok::r_brace,
                       tok::numeric_constant) ||
          Keywords.isWordLike(Left))) {
       return false;
     }
     // Add space in attribute like `(* ASYNC_REG = "TRUE" *)`.
     if (Left.endsSequence(tok::star, tok::l_paren) && Right.is(tok::identifier))
       return true;
+    // Add space before drive strength like in `wire (strong1, pull0)`.
+    if (Right.is(tok::l_paren) && Right.is(TT_VerilogStrength))
+      return true;
   }
   if (Left.is(TT_ImplicitStringLiteral))
     return Right.hasWhitespaceBefore();
   if (Line.Type == LT_ObjCMethodDecl) {
     if (Left.is(TT_ObjCMethodSpecifier))
       return true;
     if (Left.is(tok::r_paren) && canBeObjCSelectorComponent(Right)) {
       // Don't space between ')' and <id> or ')' and 'new'. 'new' is not a
if (Right.is(tok::r_brace) && Left.is(tok::l_brace) &&
                   FormatStyle::SFS_InlineOnly);
     }
   } else if (Style.Language == FormatStyle::LK_Java) {
     if (Right.is(tok::plus) && Left.is(tok::string_literal) && Right.Next &&
         Right.Next->is(tok::string_literal)) {
       return true;
     }
   } else if (Style.isVerilog()) {
+    // Break between ports of different types.
+    if (Left.is(TT_VerilogTypeComma))
+      return true;
     // Break after labels. In Verilog labels don't have the 'case' keyword, so
     // it is hard to identify them in UnwrappedLineParser.
     if (!Keywords.isVerilogBegin(Right) && Keywords.isVerilogEndOfLabel(Left))
       return true;
   } else if (Style.Language == FormatStyle::LK_Cpp ||
              Style.Language == FormatStyle::LK_ObjC ||
              Style.Language == FormatStyle::LK_Proto ||
              Style.Language == FormatStyle::LK_TableGen ||

clang/unittests/Format/FormatTestVerilog.cpp

TEST_F(FormatTestVerilog, Case) {
   verifyFormat("case (data)\n"
                "  16'd0: begin\n"
                "    result = 10'b0111111111;\n"
                "  end\n"
                "endcase",
                Style);
 }
 
+TEST_F(FormatTestVerilog, Declaration) {
+  verifyFormat("wire mynet;");
+  verifyFormat("wire mynet, mynet1;");
+  verifyFormat("wire mynet, //\n"
+               "     mynet1;");
+  verifyFormat("wire mynet = enable;");
+  verifyFormat("wire mynet = enable, mynet1;");
+  verifyFormat("wire mynet = enable, //\n"
+               "     mynet1;");
+  verifyFormat("wire mynet, mynet1 = enable;");
+  verifyFormat("wire mynet, //\n"
+               "     mynet1 = enable;");
+  verifyFormat("wire mynet = enable, mynet1 = enable;");
+  verifyFormat("wire mynet = enable, //\n"
+               "     mynet1 = enable;");
+  verifyFormat("wire (strong1, pull0) mynet;");
+  verifyFormat("wire (strong1, pull0) mynet, mynet1;");
+  verifyFormat("wire (strong1, pull0) mynet, //\n"
+               "                      mynet1;");
+  verifyFormat("wire (strong1, pull0) mynet = enable;");
+  verifyFormat("wire (strong1, pull0) mynet = enable, mynet1;");
+  verifyFormat("wire (strong1, pull0) mynet = enable, //\n"
+               "                      mynet1;");
+  verifyFormat("wire (strong1, pull0) mynet, mynet1 = enable;");
+  verifyFormat("wire (strong1, pull0) mynet, //\n"
+               "                      mynet1 = enable;");
+}
+
 TEST_F(FormatTestVerilog, Delay) {
   // Delay by the default unit.
   verifyFormat("#0;");
   verifyFormat("#1;");
   verifyFormat("#10;");
   verifyFormat("#1.5;");
   // Explicit unit.
   verifyFormat("#1fs;");
   verifyFormat("#1.5fs;");
   verifyFormat("#1ns;");
   verifyFormat("#1.5ns;");
   verifyFormat("#1us;");
   verifyFormat("#1.5us;");
   verifyFormat("#1ms;");
   verifyFormat("#1.5ms;");
   verifyFormat("#1s;");
   verifyFormat("#1.5s;");
   // The following expression should be on the same line.
   verifyFormat("#1 x = x;");
   EXPECT_EQ("#1 x = x;", format("#1\n"
                                 "x = x;"));
 }
 
+TEST_F(FormatTestVerilog, Headers) {
+  // Test headers with multiple ports.
+  verifyFormat("module mh1\n"
+               "    (input var int in1,\n"
+               "     input var shortreal in2,\n"
+               "     output tagged_st out);\n"
+               "endmodule");
+  // Ports should be grouped by types.
+  verifyFormat("module test\n"
+               "    (input [7 : 0] a,\n"
+               "     input signed [7 : 0] b, c, d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input [7 : 0] a,\n"
+               "     (* x = x *) input signed [7 : 0] b, c, d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input [7 : 0] a = 0,\n"
+               "     input signed [7 : 0] b = 0, c = 0, d = 0);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    #(parameter x)\n"
+               "    (input [7 : 0] a,\n"
+               "     input signed [7 : 0] b, c, d);\n"
+               "endmodule");
+  // When a line needs to be broken, ports of the same type should be aligned to
+  // the same column.
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b, c, //\n"
+               "                          d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    ((* x = x *) input signed [7 : 0] b, c, //\n"
+               "                                      d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b = 0, c, //\n"
+               "                          d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b, c = 0, //\n"
+               "                          d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b, c, //\n"
+               "                          d = 0);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input wire logic signed [7 : 0][0 : 1] b, c, //\n"
+               "                                            d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b, //\n"
+               "                          c, //\n"
+               "                          d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input [7 : 0] a,\n"
+               "     input signed [7 : 0] b, //\n"
+               "                          c, //\n"
+               "                          d);\n"
+               "endmodule");
+  verifyFormat("module test\n"
+               "    (input signed [7 : 0] b, //\n"
+               "                          c, //\n"
+               "                          d,\n"
+               "     output signed [7 : 0] h);\n"
+               "endmodule");
+  // With a modport.
+  verifyFormat("module m\n"
+               "    (i2.master i);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2.master i, ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2.master i, //\n"
+               "               ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2.master i,\n"
+               "     input ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2.master i);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2.master i, ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2.master i, //\n"
+               "                   ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2.master i,\n"
+               "     input ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2 i);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2 i, ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2 i, //\n"
+               "            ii);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (i2::i2 i,\n"
+               "     input ii);\n"
+               "endmodule");
+  // With a macro in the names.
+  verifyFormat("module m\n"
+               "    (input var `x a, b);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (input var `x a, //\n"
+               "                  b);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (input var x `a, b);\n"
+               "endmodule");
+  verifyFormat("module m\n"
+               "    (input var x `a, //\n"
+               "                 b);\n"
+               "endmodule");
+  // With a concatenation in the names.
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 40;
+  verifyFormat("`define X(x)                           \\\n"
+               "  module test                          \\\n"
+               "      (input var x``x a, b);",
+               Style);
+  verifyFormat("`define X(x)                           \\\n"
+               "  module test                          \\\n"
+               "      (input var x``x aaaaaaaaaaaaaaa, \\\n"
+               "                      b);",
+               Style);
+  verifyFormat("`define X(x)                           \\\n"
+               "  module test                          \\\n"
+               "      (input var x a``x, b);",
+               Style);
+  verifyFormat("`define X(x)                           \\\n"
+               "  module test                          \\\n"
+               "      (input var x aaaaaaaaaaaaaaa``x, \\\n"
+               "                   b);",
+               Style);
+}
+
 TEST_F(FormatTestVerilog, Hierarchy) {
   verifyFormat("module x;\n"
                "endmodule");
   // Test that the end label is on the same line as the end keyword.
   verifyFormat("module x;\n"
                "endmodule : x");
   // Test that things inside are indented.
   verifyFormat("module x;\n"