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

[sstwcw via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG369e8762b4d6: [clang-format] Stop comment disrupting 
indentation of Verilog ports (authored by sstwcw).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTestVerilog.cpp

Index: clang/unittests/Format/FormatTestVerilog.cpp
===
--- clang/unittests/Format/FormatTestVerilog.cpp
+++ clang/unittests/Format/FormatTestVerilog.cpp
@@ -517,6 +517,15 @@
"(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;
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -4196,11 +4196,14 @@
 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();
 }
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3312,6 +3312,8 @@
   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;
   }
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -156,6 +156,9 @@
   /* 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)` */\
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -748,7 +748,8 @@
   !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;
   }
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -4019,9 +4019,12 @@
   /// 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 usa

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

> 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};


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

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




Comment at: clang/include/clang/Format/Format.h:4027
+  /// is probably for the port on the following line instead of the parenthesis
+  /// it follows.
   /// \code

MyDeveloperDay wrote:
> This seems an odd corner case
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:4027
+  /// is probably for the port on the following line instead of the parenthesis
+  /// it follows.
   /// \code

This seems an odd corner case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149562/new/

https://reviews.llvm.org/D149562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
sstwcw requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149562

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTestVerilog.cpp

Index: clang/unittests/Format/FormatTestVerilog.cpp
===
--- clang/unittests/Format/FormatTestVerilog.cpp
+++ clang/unittests/Format/FormatTestVerilog.cpp
@@ -484,6 +484,15 @@
"(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;
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -4194,11 +4194,14 @@
 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();
 }
 
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3312,6 +3312,8 @@
   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;
   }
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -156,6 +156,9 @@
   /* 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)` */\
Index: clang/lib/Format/ContinuationIndenter.cpp
===