Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format] Add spaces around the Verilog implication operator #71352

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

sstwcw
Copy link
Contributor

@sstwcw sstwcw commented Nov 6, 2023

The Verilog implication operator -> is a binary operator meaning either the left hand side is false or the right hand side is true. Previously it was treated as the C++ struct member operator.

I didn't even know it existed when I added the operator formatting part. And I didn't check all the tests for all the operators I added. That is how the bad test got in.

The Verilog implication operator `->` is a binary operator meaning
either the left hand side is false or the right hand side is true.
Previously it was treated as the C++ struct member operator.

I didn't even know it existed when I added the operator formatting part.
And I didn't check all the tests for all the operators I added.  That is
how the bad test got in.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 6, 2023

@llvm/pr-subscribers-clang-format

Author: None (sstwcw)

Changes

The Verilog implication operator -> is a binary operator meaning either the left hand side is false or the right hand side is true. Previously it was treated as the C++ struct member operator.

I didn't even know it existed when I added the operator formatting part. And I didn't check all the tests for all the operators I added. That is how the bad test got in.


Full diff: https://github.com/llvm/llvm-project/pull/71352.diff

4 Files Affected:

  • (modified) clang/lib/Format/FormatTokenLexer.cpp (+4-2)
  • (modified) clang/lib/Format/TokenAnnotator.cpp (+6)
  • (modified) clang/unittests/Format/FormatTestVerilog.cpp (+1-1)
  • (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+12-11)
diff --git a/clang/lib/Format/FormatTokenLexer.cpp b/clang/lib/Format/FormatTokenLexer.cpp
index a90ba4af2da8408..db52add50a97763 100644
--- a/clang/lib/Format/FormatTokenLexer.cpp
+++ b/clang/lib/Format/FormatTokenLexer.cpp
@@ -253,7 +253,8 @@ void FormatTokenLexer::tryMergePreviousTokens() {
                           TT_BinaryOperator)) {
       return;
     }
-    // Module paths in specify blocks and implications in properties.
+    // Module paths in specify blocks and the implication and boolean equality
+    // operators.
     if (tryMergeTokensAny({{tok::plusequal, tok::greater},
                            {tok::plus, tok::star, tok::greater},
                            {tok::minusequal, tok::greater},
@@ -265,7 +266,8 @@ void FormatTokenLexer::tryMergePreviousTokens() {
                            {tok::pipe, tok::arrow},
                            {tok::hash, tok::minus, tok::hash},
                            {tok::hash, tok::equal, tok::hash}},
-                          TT_BinaryOperator)) {
+                          TT_BinaryOperator) ||
+        Tokens.back()->is(tok::arrow)) {
       Tokens.back()->ForcedPrecedence = prec::Comma;
       return;
     }
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 729e7e370bf62ea..39dac2917d2063c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2010,6 +2010,9 @@ class AnnotatingParser {
     } else if (Current.is(tok::arrow) &&
                Style.Language == FormatStyle::LK_Java) {
       Current.setType(TT_TrailingReturnArrow);
+    } else if (Current.is(tok::arrow) && Style.isVerilog()) {
+      // The implication operator.
+      Current.setType(TT_BinaryOperator);
     } else if (Current.is(tok::arrow) && AutoFound &&
                Line.MightBeFunctionDecl && Current.NestingLevel == 0 &&
                !Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
@@ -4684,6 +4687,9 @@ bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
         (Left.is(TT_VerilogNumberBase) && Right.is(tok::numeric_constant))) {
       return false;
     }
+    // Add spaces around the implication operator `->`.
+    if (Left.is(tok::arrow) || Right.is(tok::arrow))
+      return true;
     // Don't add spaces between two at signs. Like in a coverage event.
     // Don't add spaces between at and a sensitivity list like
     // `@(posedge clk)`.
diff --git a/clang/unittests/Format/FormatTestVerilog.cpp b/clang/unittests/Format/FormatTestVerilog.cpp
index 1c2692467987d9b..6650caea80b0524 100644
--- a/clang/unittests/Format/FormatTestVerilog.cpp
+++ b/clang/unittests/Format/FormatTestVerilog.cpp
@@ -1005,7 +1005,7 @@ TEST_F(FormatTestVerilog, Operators) {
   verifyFormat("x = x ^~ x;");
   verifyFormat("x = x && x;");
   verifyFormat("x = x || x;");
-  verifyFormat("x = x->x;");
+  verifyFormat("x = x -> x;");
   verifyFormat("x = x <-> x;");
   verifyFormat("x += x;");
   verifyFormat("x -= x;");
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index ed730307074963f..c797ddb367086d5 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1980,17 +1980,18 @@ TEST_F(TokenAnnotatorTest, UnderstandsVerilogOperators) {
   // joined operators, we don't have a separate type, so we only test for their
   // precedence.
   std::pair<prec::Level, std::string> JoinedBinary[] = {
-      {prec::Comma, "<->"},       {prec::Assignment, "+="},
-      {prec::Assignment, "-="},   {prec::Assignment, "*="},
-      {prec::Assignment, "/="},   {prec::Assignment, "%="},
-      {prec::Assignment, "&="},   {prec::Assignment, "^="},
-      {prec::Assignment, "<<="},  {prec::Assignment, ">>="},
-      {prec::Assignment, "<<<="}, {prec::Assignment, ">>>="},
-      {prec::LogicalOr, "||"},    {prec::LogicalAnd, "&&"},
-      {prec::Equality, "=="},     {prec::Equality, "!="},
-      {prec::Equality, "==="},    {prec::Equality, "!=="},
-      {prec::Equality, "==?"},    {prec::Equality, "!=?"},
-      {prec::ExclusiveOr, "~^"},  {prec::ExclusiveOr, "^~"},
+      {prec::Comma, "->"},        {prec::Comma, "<->"},
+      {prec::Assignment, "+="},   {prec::Assignment, "-="},
+      {prec::Assignment, "*="},   {prec::Assignment, "/="},
+      {prec::Assignment, "%="},   {prec::Assignment, "&="},
+      {prec::Assignment, "^="},   {prec::Assignment, "<<="},
+      {prec::Assignment, ">>="},  {prec::Assignment, "<<<="},
+      {prec::Assignment, ">>>="}, {prec::LogicalOr, "||"},
+      {prec::LogicalAnd, "&&"},   {prec::Equality, "=="},
+      {prec::Equality, "!="},     {prec::Equality, "==="},
+      {prec::Equality, "!=="},    {prec::Equality, "==?"},
+      {prec::Equality, "!=?"},    {prec::ExclusiveOr, "~^"},
+      {prec::ExclusiveOr, "^~"},
   };
   for (auto Operator : JoinedBinary) {
     auto Tokens = Annotate(std::string("x = x ") + Operator.second + " x;");

{prec::Equality, "==="}, {prec::Equality, "!=="},
{prec::Equality, "==?"}, {prec::Equality, "!=?"},
{prec::ExclusiveOr, "~^"}, {prec::ExclusiveOr, "^~"},
{prec::Comma, "->"}, {prec::Comma, "<->"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding it at front and generating a huge diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <-> was previously in the front. And the -> is related. So I put them next to each other.

@sstwcw sstwcw merged commit 3af82b3 into llvm:main Nov 29, 2023
4 checks passed
@sstwcw sstwcw deleted the format-verilog-implies branch November 29, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants