[clang-format] Stop comment disrupting indentation of Verilog ports

[sstwcw]

Summary

Before:

module x
    #( //
        parameter x)
    ( //
        input y);
endmodule

After:

module x
    #(//
      parameter x)
    (//
     input y);
endmodule

If the first line in a port or parameter list is not a comment, the
following lines would be aligned to the first line as intended:

module x
    #(parameter x1,
      parameter x2)
    (input y,
     input y2);
endmodule

Previously, the indentation would be changed to an extra continuation
indentation relative to the start of the parenthesis or the hash if
the first token inside the parentheses is a comment. It is a feature
introduced in ddaa9be97839. The feature enabled one to insert a //
comment right after an opening parentheses to put the function
arguments on a new line with a small indentation regardless of how
long the function name is, like this:

someFunction(anotherFunction( // Force break.
    parameter));

People are unlikely to use this feature in a Verilog port list because
the formatter already puts the port list on its own lines. A comment
at the start of a port list is probably a comment for the port on the
next line.

We also removed the space before the comment so that its indentation
would be same as that for a line comment anywhere else in the port
list.

[MyDeveloperDay]

clang/include/clang/Format/Format.h

4027

This seems an odd corner case

[HazardyKnusperkeks]

I don't see the problem, could you elaborate a bit more? (Keep in mind, I have no idea about Verilog.)

[sstwcw]

In D149562#4310396, @HazardyKnusperkeks wrote:

I don't see the problem, could you elaborate a bit more? (Keep in mind, I have no idea about Verilog.)

The current way the port list gets formatted is like this:

module x
    (input x1,
     input x2,
     input x3,
     input x4);
endmodule

One may want to add a comment for one of the ports like this:

module x
    (input x1,
     // second port
     input x2,
     // third port
     input x3,
     // forth port
     input x4);
endmodule

The port list thing and the comment thing work fine for now, except
when there is a comment for the first port. The comment on the first
line would cause the port list to be indented, like this:

module x
    ( // first port
        input x1,
        // second port
        input x2,
        // third port
        input x3,
        // forth port
        input x4);
endmodule

After this patch, a comment on the first line would not cause the
problem. Now the code gets formatted like this. The ports are
indented the same whether or not there is a comment on the first line.

module x
    (// first port
     input x1,
     // second port
     input x2,
     // third port
     input x3,
     // forth port
     input x4);
endmodule

clang/include/clang/Format/Format.h

4027

See the last block of code in the example I added. This is the only way I came up with to ensure that the comment for the first port is aligned with the comments for the rest of the ports and that whether the first comment exists does not affect how other lines are indented.

[owenpan]

In D149562#4312573, @sstwcw wrote:

The port list thing and the comment thing work fine for now, except
when there is a comment for the first port. The comment on the first
line would cause the port list to be indented, like this:

module x
    ( // first port
        input x1,
        // second port
        input x2,
        // third port
        input x3,
        // forth port
        input x4);
endmodule

After this patch, a comment on the first line would not cause the
problem. Now the code gets formatted like this. The ports are
indented the same whether or not there is a comment on the first line.

module x
    (// first port
     input x1,
     // second port
     input x2,
     // third port
     input x3,
     // forth port
     input x4);
endmodule

See https://github.com/llvm/llvm-project/issues/55487#issuecomment-1321355199, which may be relevant.

[sstwcw]

IMO a trailing comment (empty or not) belongs to the code before it.

There is only a parenthesis before it. It doesn't usually need a comment. It is like in 5a61139. One doesn't tend to comment a single brace.

A comment about the code below it should start on a new line.

In this special case, the comment would be indented to the same column as the next line, so it should be clear it is for the next line. Like the case forbraced initializer lists, the first field will be on the same line as the brace if there isn't a comment, so the first comment is also on the same line as the brace when there is one.

foo foo{// first field
        a,
        // second field
        b};

[owenpan]

In D149562#4332188, @sstwcw wrote:

IMO a trailing comment (empty or not) belongs to the code before it.

There is only a parenthesis before it. It doesn't usually need a comment. It is like in 5a61139. One doesn't tend to comment a single brace.

I agree if the brace doesn't start a block, but clang-format sometimes got it wrong and misannotates a block as a braced list. (See https://github.com/llvm/llvm-project/issues/33891 for an example.)

A comment about the code below it should start on a new line.

In this special case, the comment would be indented to the same column as the next line, so it should be clear it is for the next line. Like the case forbraced initializer lists, the first field will be on the same line as the brace if there isn't a comment, so the first comment is also on the same line as the brace when there is one.

foo foo{// first field
        a,
        // second field
        b};

I'll go along with other reviewers on this one.

[sstwcw]

I agree if the brace doesn't start a block, but clang-format sometimes got it wrong and misannotates a block as a braced list. (See https://github.com/llvm/llvm-project/issues/33891 for an example.)

This patch isn't affected by the problem. The Verilog port list is not as ambiguous as braced blocks.

I'll go along with other reviewers on this one.

So what do the other reviewers think about this patch? The braced initialization list thing was added a long time ago by DJasper, so that behavior probably has to stay.

[HazardyKnusperkeks]

In D149562#4337955, @sstwcw wrote:

I'll go along with other reviewers on this one.

So what do the other reviewers think about this patch? The braced initialization list thing was added a long time ago by DJasper, so that behavior probably has to stay.

I'm not particularly fond of that feature, but you are right, it has to stay. So making it consistent in verilog doesn't seem to be a problem to me.

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
      }                                    }
 
 .. _SpacesBeforeTrailingComments:
 
 **SpacesBeforeTrailingComments** (``Unsigned``) :versionbadge:`clang-format 3.7` :ref:`¶ <SpacesBeforeTrailingComments>`
   The number of spaces before trailing line comments
   (``//`` - comments).
 
-  This does not affect trailing block comments (``/*`` - comments) as
-  those commonly have different usage patterns and a number of special
-  cases.
+  This does not affect trailing block comments (``/*`` - comments) as those
+  commonly have different usage patterns and a number of special cases.  In
+  the case of Verilog, it doesn't affect a comment right after the opening
+  parenthesis in the port or parameter list in a module header, because it
+  is probably for the port on the following line instead of the parenthesis
+  it follows.
 
   .. code-block:: c++
 
      SpacesBeforeTrailingComments: 3
      void f() {
        if (true) {   // foo1
          f();        // bar
        }             // foo

clang/include/clang/Format/Format.h

   ///    }                                    }
   /// \endcode
   /// \version 3.7
   bool SpaceInEmptyParentheses;
 
   /// The number of spaces before trailing line comments
   /// (``//`` - comments).
   ///
-  /// This does not affect trailing block comments (``/*`` - comments) as
-  /// those commonly have different usage patterns and a number of special
-  /// cases.
+  /// This does not affect trailing block comments (``/*`` - comments) as those
+  /// commonly have different usage patterns and a number of special cases.  In
+  /// the case of Verilog, it doesn't affect a comment right after the opening
+  /// parenthesis in the port or parameter list in a module header, because it
+  /// is probably for the port on the following line instead of the parenthesis
+  /// it follows.
   /// \code
   ///    SpacesBeforeTrailingComments: 3
   ///    void f() {
   ///      if (true) {   // foo1
   ///        f();        // bar
   ///      }             // foo
   ///    }
   /// \endcode

clang/lib/Format/ContinuationIndenter.cpp

void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
   // Align following lines within parentheses / brackets if configured.
   // Note: This doesn't apply to macro expansion lines, which are MACRO( , , )
   // with args as children of the '(' and ',' tokens. It does not make sense to
   // align the commas with the opening paren.
   if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
       !CurrentState.IsCSharpGenericTypeConstraint && Previous.opensScope() &&
       Previous.isNot(TT_ObjCMethodExpr) && Previous.isNot(TT_RequiresClause) &&
       !(Current.MacroParent && Previous.MacroParent) &&
-      (Current.isNot(TT_LineComment) || Previous.is(BK_BracedInit))) {
+      (Current.isNot(TT_LineComment) ||
+       Previous.isOneOf(BK_BracedInit, TT_VerilogMultiLineListLParen))) {
     CurrentState.Indent = State.Column + Spaces;
     CurrentState.IsAligned = true;
   }
   if (CurrentState.AvoidBinPacking && startsNextParameter(Current, Style))
     CurrentState.NoLineBreak = true;
   if (startsSegmentOfBuilderTypeCall(Current) &&
       State.Column > getNewLineColumn(State)) {
     CurrentState.ContainsUnwrappedBuilder = true;

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   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)                                              \
+  /* A parenthesized list within which line breaks are inserted by the         \
+   * formatter, for example the list of ports in a module header. */           \
+  TYPE(VerilogMultiLineListLParen)                                             \
   /* 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

void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
   }
 
   while (Current) {
     const FormatToken *Prev = Current->Previous;
     if (Current->is(TT_LineComment)) {
       if (Prev->is(BK_BracedInit) && Prev->opensScope()) {
         Current->SpacesRequiredBefore =
             (Style.Cpp11BracedListStyle && !Style.SpacesInParentheses) ? 0 : 1;
+      } else if (Prev->is(TT_VerilogMultiLineListLParen)) {
+        Current->SpacesRequiredBefore = 0;
       } else {
         Current->SpacesRequiredBefore = Style.SpacesBeforeTrailingComments;
       }
 
       // If we find a trailing comment, iterate backwards to determine whether
       // it seems to relate to a specific parameter. If so, break before that
       // parameter to avoid changing the comment's meaning. E.g. don't move 'b'
       // to the previous line in:

clang/lib/Format/UnwrappedLineParser.cpp

while (FormatTok->is(Keywords.kw_import)) {
       if (FormatTok->is(tok::semi))
         nextToken();
     }
 
     // parameters and ports
     if (FormatTok->is(Keywords.kw_verilogHash)) {
       NewLine();
       nextToken();
-      if (FormatTok->is(tok::l_paren))
+      if (FormatTok->is(tok::l_paren)) {
+        FormatTok->setFinalizedType(TT_VerilogMultiLineListLParen);
         parseParens();
       }
+    }
     if (FormatTok->is(tok::l_paren)) {
       NewLine();
+      FormatTok->setFinalizedType(TT_VerilogMultiLineListLParen);
       parseParens();
     }
 
     // extends and implements
     if (FormatTok->is(Keywords.kw_extends)) {
       NewLine();
       nextToken();
       parseVerilogHierarchyIdentifier();

clang/unittests/Format/FormatTestVerilog.cpp

verifyFormat("module m\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");
+  // A line comment shouldn't disrupt the indentation of the port list.
+  verifyFormat("extern module x\n"
+               "    (//\n"
+               "     output y);");
+  verifyFormat("extern module x\n"
+               "    #(//\n"
+               "      parameter x)\n"
+               "    (//\n"
+               "     output y);");
   // With a concatenation in the names.
   auto Style = getDefaultStyle();
   Style.ColumnLimit = 40;
   verifyFormat("`define X(x)                           \\\n"
                "  module test                          \\\n"
                "      (input var x``x a, b);",
                Style);
   verifyFormat("`define X(x)                           \\\n"