[clang-format] Add option for having one port per line in Verilog

[sstwcw]

Summary

We added the option VerilogBreakBetweenInstancePorts to put ports on
separate lines in module instantiations. We made it default to true
because style guides mostly recommend it that way for example:

https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#module-instantiation

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

[sstwcw]

Generate doc

[HazardyKnusperkeks]

clang/include/clang/Format/Format.h

4185

Can you put the true vs. false in the doc, like on other options?

4192

V after U

clang/lib/Format/TokenAnnotator.cpp

1150

1156–1163

I won't read that.

I think you should go with multiple if statements.

[sstwcw]

• Use shorter conditions

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

1149–1191

Can you make it a function or lambda with early returns?

[sstwcw]

• Use lambda

clang/include/clang/Format/Format.h

enum UseTabStyle : int8_t {
     /// Use tabs for line continuation and indentation, and spaces for
     /// alignment.
     UT_AlignWithSpaces,
     /// Use tabs whenever we need to fill whitespace that spans at least from
     /// one tab stop to the next one.
     UT_Always
   };
 
+  /// For Verilog, put each port on its own line in module instantiations.
+  /// \code
+  ///    ffnand ff1(.q(),
+  ///               .qbar(out1),
+  ///               .clear(in1),
+  ///               .preset(in2));
+  /// \endcode
+  /// \version 17
+  bool VerilogBreakBetweenInstancePorts;
+
   /// The way to use tab characters in the resulting file.
   /// \version 3.7
   UseTabStyle UseTab;
 
   /// A vector of macros which are whitespace-sensitive and should not
   /// be touched.
   ///
   /// These are expected to be macros of the form:
return AccessModifierOffset == R.AccessModifierOffset &&
            SpacesInLineCommentPrefix.Maximum ==
                R.SpacesInLineCommentPrefix.Maximum &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
            Standard == R.Standard &&
            StatementAttributeLikeMacros == R.StatementAttributeLikeMacros &&
            StatementMacros == R.StatementMacros && TabWidth == R.TabWidth &&
            TypenameMacros == R.TypenameMacros && UseTab == R.UseTab &&
+           VerilogBreakBetweenInstancePorts ==
+               R.VerilogBreakBetweenInstancePorts &&
            WhitespaceSensitiveMacros == R.WhitespaceSensitiveMacros;
   }
 
   std::optional<FormatStyle> GetLanguageStyle(LanguageKind Language) const;
 
   // Stores per-language styles. A FormatStyle instance inside has an empty
   // StyleSet. A FormatStyle instance returned by the Get method has its
   // StyleSet set to a copy of the originating StyleSet, effectively keeping the

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("StatementAttributeLikeMacros",
                    Style.StatementAttributeLikeMacros);
     IO.mapOptional("StatementMacros", Style.StatementMacros);
     IO.mapOptional("TabWidth", Style.TabWidth);
     IO.mapOptional("TypenameMacros", Style.TypenameMacros);
     IO.mapOptional("UseTab", Style.UseTab);
+    IO.mapOptional("VerilogBreakBetweenInstancePorts",
+                   Style.VerilogBreakBetweenInstancePorts);
     IO.mapOptional("WhitespaceSensitiveMacros",
                    Style.WhitespaceSensitiveMacros);
     IO.mapOptional("Macros", Style.Macros);
 
     // If AlwaysBreakAfterDefinitionReturnType was specified but
     // AlwaysBreakAfterReturnType was not, initialize the latter from the
     // former for backwards compatibility.
     if (Style.AlwaysBreakAfterDefinitionReturnType != FormatStyle::DRTBS_None &&
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpacesInSquareBrackets = false;
   LLVMStyle.SpacesInConditionalStatement = false;
   LLVMStyle.Standard = FormatStyle::LS_Latest;
   LLVMStyle.StatementAttributeLikeMacros.push_back("Q_EMIT");
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
   LLVMStyle.TabWidth = 8;
   LLVMStyle.UseTab = FormatStyle::UT_Never;
+  LLVMStyle.VerilogBreakBetweenInstancePorts = true;
   LLVMStyle.WhitespaceSensitiveMacros.push_back("BOOST_PP_STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("CF_SWIFT_NAME");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("NS_SWIFT_NAME");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("PP_STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("STRINGIZE");
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   /* Like in 'assign x = 0, y = 1;' . */                                       \
   TYPE(VerilogAssignComma)                                                     \
   /* 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)                                             \
+  /* list of port connections or parameters in a module instantiation */       \
+  TYPE(VerilogInstancePortComma)                                               \
+  TYPE(VerilogInstancePortLParen)                                              \
   /* 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)                                                       \

clang/lib/Format/TokenAnnotator.cpp

if (OpeningParen.is(TT_OverloadedOperatorLParen)) {
       }
 
       // If faced with "a.operator*(argument)" or "a->operator*(argument)",
       // i.e. the operator is called as a member function,
       // then the argument must be an expression.
       bool OperatorCalledAsMemberFunction =
           Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
       Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
+    } else if (OpeningParen.is(TT_VerilogInstancePortLParen)) {
+      Contexts.back().IsExpression = true;
+      Contexts.back().ContextType = Context::VerilogInstancePortList;
     } else if (Style.isJavaScript() &&
                (Line.startsWith(Keywords.kw_type, tok::identifier) ||
                 Line.startsWith(tok::kw_export, Keywords.kw_type,
                                 tok::identifier))) {
       // type X = (...);
       // export type X = (...);
       Contexts.back().IsExpression = false;
     } else if (OpeningParen.Previous &&
case tok::l_paren:
       if (Tok->Previous && Tok->Previous->is(tok::r_paren) &&
           Tok->Previous->MatchingParen &&
           Tok->Previous->MatchingParen->is(TT_OverloadedOperatorLParen)) {
         Tok->Previous->setType(TT_OverloadedOperator);
         Tok->Previous->MatchingParen->setType(TT_OverloadedOperator);
         Tok->setType(TT_OverloadedOperatorLParen);
       }
 
+      if (Style.isVerilog()) {
+        const FormatToken *Prev = Tok->getPreviousNonComment(), *Prev2;
+        // Identify the parameter list and port list in a module instantiation.
+        // This is still needed when we already have
+        // UnwrappedLineParser::parseVerilogHierarchyHeader because that
+        // function is only responsible for the definition, not the
+        // instantiation.
+        if (Prev && (Prev2 = Prev->getPreviousNonComment()) &&
+            ((Prev->is(tok::hash) && Keywords.isVerilogIdentifier(*Prev2)) ||
+             (Keywords.isVerilogIdentifier(*Prev) &&
+              (Prev2->is(tok::r_paren) ||
+               Keywords.isVerilogIdentifier(*Prev2) ||
+               (Prev2->endsSequence(tok::comma, tok::r_paren) &&
+                (Prev2 = Prev2->getPreviousNonComment()->MatchingParen) &&
+                Prev2->is(TT_VerilogInstancePortLParen)))))) {
+          Tok->setFinalizedType(TT_VerilogInstancePortLParen);
+        }
+      }
+
       if (!parseParens())
         return false;
       if (Line.MustBeDeclaration && Contexts.size() == 1 &&
           !Contexts.back().IsExpression && !Line.startsWith(TT_ObjCProperty) &&
           !Tok->isOneOf(TT_TypeDeclarationParen, TT_RequiresExpressionLParen) &&
           (!Tok->Previous ||
            !Tok->Previous->isOneOf(tok::kw___attribute, TT_RequiresClause,
                                    TT_LeadingJavaAnnotation))) {
         Line.MightBeFunctionDecl = true;
       }
       break;
     case tok::l_square:
       if (!parseSquare())
         return false;
       break;
     case tok::l_brace:
       if (Style.Language == FormatStyle::LK_TextProto) {
         FormatToken *Previous = Tok->getPreviousNonComment();
         if (Previous && Previous->getType() != TT_DictLiteral)
           Previous->setType(TT_SelectorName);
       }
       Scopes.push_back(getScopeType(*Tok));
       if (!parseBrace())
         return false;
       break;
     case tok::less:
       if (parseAngle()) {
         Tok->setType(TT_TemplateOpener);
         // In TT_Proto, we must distignuish between:
         //   map<key, value>
         //   msg < item: data >
         //   msg: < item: data >
bool consumeToken() {
     case tok::comma:
       switch (Contexts.back().ContextType) {
       case Context::CtorInitializer:
         Tok->setType(TT_CtorInitializerComma);
         break;
       case Context::InheritanceList:
         Tok->setType(TT_InheritanceComma);
         break;
+      case Context::VerilogInstancePortList:
+        Tok->setFinalizedType(TT_VerilogInstancePortComma);
+        break;
       default:
         if (Style.isVerilog() && Contexts.size() == 1 &&
             Line.startsWith(Keywords.kw_assign)) {
           Tok->setFinalizedType(TT_VerilogAssignComma);
         } else if (Contexts.back().FirstStartOfName &&
                    (Contexts.size() == 1 || startsWithInitStatement(Line))) {
           Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
           Line.IsMultiVariableDeclStmt = true;
enum {
       InheritanceList,
       // Like in the braced list.
       //   int x[] = {};
       StructArrayInitializer,
       // Like in `static_cast<int>`.
       TemplateArgument,
       // C11 _Generic selection.
       C11GenericSelection,
+      // Like in the outer parentheses in `ffnand ff1(.q());`.
+      VerilogInstancePortList,
     } ContextType = Unknown;
   };
 
   /// Puts a new \c Context onto the stack \c Contexts for the lifetime
   /// of each instance.
   struct ScopedContextCreator {
     AnnotatingParser &P;
 
if (Style.isCSharp()) {
     }
   } else if (Style.isVerilog()) {
     // Break between assignments.
     if (Left.is(TT_VerilogAssignComma))
       return true;
     // Break between ports of different types.
     if (Left.is(TT_VerilogTypeComma))
       return true;
+    // Break between ports in a module instantiation and after the parameter
+    // list.
+    if (Style.VerilogBreakBetweenInstancePorts &&
+        (Left.is(TT_VerilogInstancePortComma) ||
+         (Left.is(tok::r_paren) && Keywords.isVerilogIdentifier(Right) &&
+          Left.MatchingParen &&
+          Left.MatchingParen->is(TT_VerilogInstancePortLParen)))) {
+      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/ConfigParseTest.cpp

TEST(ConfigParseTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCaseColon);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeJsonColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
+  CHECK_PARSE_BOOL(VerilogBreakBetweenInstancePorts);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterStruct);

clang/unittests/Format/FormatTestVerilog.cpp

TEST_F(FormatTestVerilog, If) {
   verifyFormat("(* x *) if (x)\n"
                "  x = x;");
   verifyFormat("(* x = \"x\" *) if (x)\n"
                "  x = x;");
   verifyFormat("(* x, x = \"x\" *) if (x)\n"
                "  x = x;");
 }
 
+TEST_F(FormatTestVerilog, Instantiation) {
+  // Without ports.
+  verifyFormat("ffnand ff1;");
+  // With named ports.
+  verifyFormat("ffnand ff1(.qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2));");
+  // With wildcard.
+  verifyFormat("ffnand ff1(.qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2),\n"
+               "           .*);");
+  verifyFormat("ffnand ff1(.*,\n"
+               "           .qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2));");
+  // With unconnected ports.
+  verifyFormat("ffnand ff1(.q(),\n"
+               "           .qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2));");
+  verifyFormat("ffnand ff1(.q(),\n"
+               "           .qbar(),\n"
+               "           .clear(),\n"
+               "           .preset());");
+  verifyFormat("ffnand ff1(,\n"
+               "           .qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2));");
+  // With positional ports.
+  verifyFormat("ffnand ff1(out1,\n"
+               "           in1,\n"
+               "           in2);");
+  verifyFormat("ffnand ff1(,\n"
+               "           out1,\n"
+               "           in1,\n"
+               "           in2);");
+  // Multiple instantiations.
+  verifyFormat("ffnand ff1(.q(),\n"
+               "           .qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2)),\n"
+               "       ff1(.q(),\n"
+               "           .qbar(out1),\n"
+               "           .clear(in1),\n"
+               "           .preset(in2));");
+  verifyFormat("ffnand //\n"
+               "    ff1(.q(),\n"
+               "        .qbar(out1),\n"
+               "        .clear(in1),\n"
+               "        .preset(in2)),\n"
+               "    ff1(.q(),\n"
+               "        .qbar(out1),\n"
+               "        .clear(in1),\n"
+               "        .preset(in2));");
+  // With breaking between instance ports disabled.
+  auto Style = getDefaultStyle();
+  Style.VerilogBreakBetweenInstancePorts = false;
+  verifyFormat("ffnand ff1;", Style);
+  verifyFormat("ffnand ff1(.qbar(out1), .clear(in1), .preset(in2), .*);",
+               Style);
+  verifyFormat("ffnand ff1(out1, in1, in2);", Style);
+  verifyFormat("ffnand ff1(.q(), .qbar(out1), .clear(in1), .preset(in2)),\n"
+               "       ff1(.q(), .qbar(out1), .clear(in1), .preset(in2));",
+               Style);
+  verifyFormat("ffnand //\n"
+               "    ff1(.q(), .qbar(out1), .clear(in1), .preset(in2)),\n"
+               "    ff1(.q(), .qbar(out1), .clear(in1), .preset(in2));",
+               Style);
+}
+
 TEST_F(FormatTestVerilog, Operators) {
   // Test that unary operators are not followed by space.
   verifyFormat("x = +x;");
   verifyFormat("x = -x;");
   verifyFormat("x = !x;");
   verifyFormat("x = ~x;");
   verifyFormat("x = &x;");
   verifyFormat("x = ~&x;");