[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2e32ff106e74: [clang-format] Handle Verilog preprocessor 
directives (authored by sstwcw).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -30,7 +30,9 @@
 return *Result;
   }
 
-  static std::string format(llvm::StringRef Code, const FormatStyle ) {
+  static std::string
+  format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(FormatStyle::LK_Verilog)) {
 return format(Code, 0, Code.size(), Style);
   }
 
@@ -43,6 +45,29 @@
   }
 };
 
+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, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +139,113 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {
+  "begin_keywords",
+  "celldefine",
+  "default_nettype",
+  "define",
+  "else",
+  "elsif",
+  "end_keywords",
+  "endcelldefine",
+  "endif",
+  "ifdef",
+  "ifndef",
+  "include",
+  "line",
+  "nounconnected_drive",
+  "pragma",
+  "resetall",
+  "timescale",
+  "unconnected_drive",
+  "undef",
+  "undefineall",
+  };
+  for (auto  : Directives) {
+EXPECT_EQ("if (x)\n"
+  "`" +
+  Name +
+  "\n"
+  "  ;",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+  // Lines starting with a regular macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+  std::string NonDirectives[] = {
+  // For `__FILE__` and `__LINE__`, although the standard classifies them as
+  // preprocessor directives, they are used like regular macros.
+  "__FILE__", "__LINE__", "elif", "foo", "x",
+  };
+  for (auto  : NonDirectives) {
+EXPECT_EQ("if (x)\n"
+  

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

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

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added inline comments.
This revision is now accepted and ready to land.
Herald added a subscriber: nlopes.



Comment at: clang/lib/Format/FormatToken.h:982
 
 kw_always = ("always");
 kw_always_comb = ("always_comb");

It seems a comment before is missing. Should be fixed in a separate patch.



Comment at: clang/lib/Format/FormatToken.h:1142
+VerilogExtraKeywords =
+std::unordered_set({kw_always,
+  kw_always_comb,

sstwcw wrote:
> Now that I added `kw_` to `verilogHashHash`, four columns can't fit.
Happens. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 3 inline comments as done.
sstwcw added inline comments.



Comment at: clang/lib/Format/FormatToken.h:1142
+VerilogExtraKeywords =
+std::unordered_set({kw_always,
+  kw_always_comb,

Now that I added `kw_` to `verilogHashHash`, four columns can't fit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 435788.
sstwcw added a comment.

- add `kw_` prefix to symbols treated as keywords
- use lambda instead of breaking


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -30,7 +30,9 @@
 return *Result;
   }
 
-  static std::string format(llvm::StringRef Code, const FormatStyle ) {
+  static std::string
+  format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(FormatStyle::LK_Verilog)) {
 return format(Code, 0, Code.size(), Style);
   }
 
@@ -43,6 +45,29 @@
   }
 };
 
+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, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +139,113 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {
+  "begin_keywords",
+  "celldefine",
+  "default_nettype",
+  "define",
+  "else",
+  "elsif",
+  "end_keywords",
+  "endcelldefine",
+  "endif",
+  "ifdef",
+  "ifndef",
+  "include",
+  "line",
+  "nounconnected_drive",
+  "pragma",
+  "resetall",
+  "timescale",
+  "unconnected_drive",
+  "undef",
+  "undefineall",
+  };
+  for (auto  : Directives) {
+EXPECT_EQ("if (x)\n"
+  "`" +
+  Name +
+  "\n"
+  "  ;",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+  // Lines starting with a regular macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+  std::string NonDirectives[] = {
+  // For `__FILE__` and `__LINE__`, although the standard classifies them as
+  // preprocessor directives, they are used like regular macros.
+  "__FILE__", "__LINE__", "elif", "foo", "x",
+  };
+  for (auto  : NonDirectives) {
+EXPECT_EQ("if (x)\n"
+  "  `" +
+ 

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1121
+  const char *Start = Lex->getBufferLocation();
+  if (!VerilogToken.match(StringRef(Start, Lex->getBuffer().end() - Start),
+  ))

Add braces. Maybe check all other added ifs too. You can check with 
clang-format with `InsertBraces` and `RemoveBracesLLVM`.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1891
+  // Verilog macro expansions begin with a backtick. For C++ we assume what
+  // looks like one is one.
   TokenCount = Line->Tokens.size();





Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1893
   TokenCount = Line->Tokens.size();
-  if (TokenCount == 1 ||
-  (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {
-if (FormatTok->is(tok::colon) && !Line->MustBeDeclaration) {
-  Line->Tokens.begin()->Tok->MustBreakBefore = true;
-  parseLabel(!Style.IndentGotoLabels);
-  if (HasLabel)
-*HasLabel = true;
-  return;
+  {
+const UnwrappedLineNode *Tok = >Tokens.front(),

I find just scopes a bit confusing. Maybe put the check if we want to break 
into a lambda and call that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 433702.
sstwcw added a comment.

- add comment and format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -30,7 +30,9 @@
 return *Result;
   }
 
-  static std::string format(llvm::StringRef Code, const FormatStyle ) {
+  static std::string
+  format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(FormatStyle::LK_Verilog)) {
 return format(Code, 0, Code.size(), Style);
   }
 
@@ -43,6 +45,29 @@
   }
 };
 
+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, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +139,113 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {
+  "begin_keywords",
+  "celldefine",
+  "default_nettype",
+  "define",
+  "else",
+  "elsif",
+  "end_keywords",
+  "endcelldefine",
+  "endif",
+  "ifdef",
+  "ifndef",
+  "include",
+  "line",
+  "nounconnected_drive",
+  "pragma",
+  "resetall",
+  "timescale",
+  "unconnected_drive",
+  "undef",
+  "undefineall",
+  };
+  for (auto  : Directives) {
+EXPECT_EQ("if (x)\n"
+  "`" +
+  Name +
+  "\n"
+  "  ;",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+  // Lines starting with a regular macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+  std::string NonDirectives[] = {
+  // For `__FILE__` and `__LINE__`, although the standard classifies them as
+  // preprocessor directives, they are used like regular macros.
+  "__FILE__", "__LINE__", "elif", "foo", "x",
+  };
+  for (auto  : NonDirectives) {
+EXPECT_EQ("if (x)\n"
+  "  `" +
+  Name + ";",
+  format("if (x)\n"
+   

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 2 inline comments as done.
sstwcw added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1889
   TokenCount = Line->Tokens.size();
-  if (TokenCount == 1 ||
-  (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {

HazardyKnusperkeks wrote:
> Why did you remove that if?
Previously the if checked whether the line is comment and an identifier.  For 
Verilog it's comment, a backtick, and an identifier. I found it easier to break 
if the line doesn't match the pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1889
   TokenCount = Line->Tokens.size();
-  if (TokenCount == 1 ||
-  (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {

Why did you remove that if?



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1906
+
 if (FormatTok->is(tok::colon) && !Line->MustBeDeclaration) {
   Line->Tokens.begin()->Tok->MustBreakBefore = true;

And now the indentation here is off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 433614.
sstwcw added a comment.

- use raw string for regex
- use default style in test
- remove parentheses
- use seek now that skipOver no longer exists


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -30,7 +30,9 @@
 return *Result;
   }
 
-  static std::string format(llvm::StringRef Code, const FormatStyle ) {
+  static std::string
+  format(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle(FormatStyle::LK_Verilog)) {
 return format(Code, 0, Code.size(), Style);
   }
 
@@ -43,6 +45,29 @@
   }
 };
 
+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, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +139,113 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {
+  "begin_keywords",
+  "celldefine",
+  "default_nettype",
+  "define",
+  "else",
+  "elsif",
+  "end_keywords",
+  "endcelldefine",
+  "endif",
+  "ifdef",
+  "ifndef",
+  "include",
+  "line",
+  "nounconnected_drive",
+  "pragma",
+  "resetall",
+  "timescale",
+  "unconnected_drive",
+  "undef",
+  "undefineall",
+  };
+  for (auto  : Directives) {
+EXPECT_EQ("if (x)\n"
+  "`" +
+  Name +
+  "\n"
+  "  ;",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+  // Lines starting with a regular macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+  std::string NonDirectives[] = {
+  // For `__FILE__` and `__LINE__`, although the standard classifies them as
+  // preprocessor directives, they are used like regular macros.
+  "__FILE__", "__LINE__", "elif", "foo", "x",
+  };
+  for (auto  : NonDirectives) {
+EXPECT_EQ("if 

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 5 inline comments as done.
sstwcw added inline comments.



Comment at: clang/unittests/Format/FormatTestVerilog.cpp:179
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {

>>>! In D124749#3490834, @MyDeveloperDay wrote:
>> You add significant number of keywords here but I don't see any of them 
>> being tested? can you add a unit tests to cover what you are adding
> 
> I think this is still open?
The keywords added in this revision are tested here. Most of those added in 
D123450 don't have tests, but I haven't added stuff to handle them yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

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

HazardyKnusperkeks added a comment.

In D124749#3514224 , @sstwcw wrote:

> The two parents of this revision change the same file, so the build bot says 
> patch does not apply.  Does that mean I have to submit the parent patches 
> with less context?

Maybe just wait until one of the parents has landed. We can make a review 
without the bot. :)
Or put the parents in relation.

In D124749#3490834 , @MyDeveloperDay 
wrote:

> You add significant number of keywords here but I don't see any of them being 
> tested? can you add a unit tests to cover what you are adding

I think this is still open?




Comment at: clang/lib/Format/FormatTokenLexer.cpp:1137
+  // normal lexer if there isn't one.
+  if (!(Style.isVerilog() && readRawTokenVerilogSpecific(Tok.Tok)))
+Lex->LexFromRawLexer(Tok.Tok);

As demonstrated by me, the parens can go under in a quick read.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1105
+  static const llvm::Regex VerilogToken(
+  "^(\'|``?|((\r?\n|\r)|[^[:space:]])*)");
+

sstwcw wrote:
> HazardyKnusperkeks wrote:
> > Consider a raw string, for a better reading.
> You mean like this?
> 
> static const llvm::Regex VerilogToken(R"re(^('|``?|\\(\\)re"
>   "(\r?\n|\r)|[^[:space:]])*)");
> 
I'd put it in one line, but basically yeah.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1129
 void FormatTokenLexer::readRawToken(FormatToken ) {
-  Lex->LexFromRawLexer(Tok.Tok);
+  if (!(Style.isVerilog() && readRawTokenVerilogSpecific(Tok.Tok)))
+Lex->LexFromRawLexer(Tok.Tok);

sstwcw wrote:
> HazardyKnusperkeks wrote:
> > Otherwise I don't see why you change that.
> The function is only supposed to be used for Verilog, so `&&` is correct. If 
> you mean why this part is different from D121758, I changed the name from 
> `readRawTokenLanguageSpecific` to `readRawTokenVerilogSpecific` as suggested 
> by a reviewer. Then I moved the check for language out of that function like 
> what `tryParseJSRegexLiteral` does.
I think I missed the parens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

The two parents of this revision change the same file, so the build bot says 
patch does not apply.  Does that mean I have to submit the parent patches with 
less context?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw added inline comments.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1105
+  static const llvm::Regex VerilogToken(
+  "^(\'|``?|((\r?\n|\r)|[^[:space:]])*)");
+

HazardyKnusperkeks wrote:
> Consider a raw string, for a better reading.
You mean like this?

static const llvm::Regex VerilogToken(R"re(^('|``?|\\(\\)re"
  "(\r?\n|\r)|[^[:space:]])*)");




Comment at: clang/lib/Format/FormatTokenLexer.cpp:1129
 void FormatTokenLexer::readRawToken(FormatToken ) {
-  Lex->LexFromRawLexer(Tok.Tok);
+  if (!(Style.isVerilog() && readRawTokenVerilogSpecific(Tok.Tok)))
+Lex->LexFromRawLexer(Tok.Tok);

HazardyKnusperkeks wrote:
> Otherwise I don't see why you change that.
The function is only supposed to be used for Verilog, so `&&` is correct. If 
you mean why this part is different from D121758, I changed the name from 
`readRawTokenLanguageSpecific` to `readRawTokenVerilogSpecific` as suggested by 
a reviewer. Then I moved the check for language out of that function like what 
`tryParseJSRegexLiteral` does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 429532.
sstwcw added a comment.

- add tests and remove __LINE__ and __FILE__ special cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -43,6 +43,30 @@
   }
 };
 
+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;",
+getLLVMStyle(FormatStyle::LK_Verilog)));
+}
+
 TEST_F(FormatTestVerilog, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +138,113 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a preprocessor directive should not be indented.
+  std::string Directives[] = {
+  "begin_keywords",
+  "celldefine",
+  "default_nettype",
+  "define",
+  "else",
+  "elsif",
+  "end_keywords",
+  "endcelldefine",
+  "endif",
+  "ifdef",
+  "ifndef",
+  "include",
+  "line",
+  "nounconnected_drive",
+  "pragma",
+  "resetall",
+  "timescale",
+  "unconnected_drive",
+  "undef",
+  "undefineall",
+  };
+  for (auto  : Directives) {
+EXPECT_EQ("if (x)\n"
+  "`" +
+  Name +
+  "\n"
+  "  ;",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+  // Lines starting with a regular macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+  std::string NonDirectives[] = {
+  // For `__FILE__` and `__LINE__`, although the standard classifies them as
+  // preprocessor directives, they are used like regular macros.
+  "__FILE__", "__LINE__", "elif", "foo", "x",
+  };
+  for (auto  : NonDirectives) {
+EXPECT_EQ("if (x)\n"
+  "  `" +
+  Name + ";",
+  format("if (x)\n"
+ "`" +
+ Name +
+ "\n"
+ ";",
+ Style));
+  }
+}
+
 } // namespace format
 } // end 

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

You add significant number of keywords here but I don't see any of them being 
tested? can you add a unit tests to cover what you are adding


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1105
+  static const llvm::Regex VerilogToken(
+  "^(\'|``?|((\r?\n|\r)|[^[:space:]])*)");
+

Consider a raw string, for a better reading.



Comment at: clang/lib/Format/FormatTokenLexer.cpp:1129
 void FormatTokenLexer::readRawToken(FormatToken ) {
-  Lex->LexFromRawLexer(Tok.Tok);
+  if (!(Style.isVerilog() && readRawTokenVerilogSpecific(Tok.Tok)))
+Lex->LexFromRawLexer(Tok.Tok);

Otherwise I don't see why you change that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

This revision depends on D124748 , but 
somehow it doesn't show up when I open the parent revision dialog.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124749

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

[PATCH] D124749: [clang-format] Handle Verilog preprocessor directives

[sstwcw via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124749

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.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
@@ -43,6 +43,30 @@
   }
 };
 
+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;",
+getLLVMStyle(FormatStyle::LK_Verilog)));
+}
+
 TEST_F(FormatTestVerilog, If) {
   verifyFormat("if (x)\n"
"  x = x;");
@@ -114,5 +138,61 @@
"  {x} = {x};");
 }
 
+TEST_F(FormatTestVerilog, Preprocessor) {
+  auto Style = getLLVMStyle(FormatStyle::LK_Verilog);
+  Style.ColumnLimit = 20;
+
+  // Macro definitions.
+  EXPECT_EQ("`define X  \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X if(x)x=x;", Style));
+  EXPECT_EQ("`define X(x)   \\\n"
+"  if (x)   \\\n"
+"x = x;",
+format("`define X(x) if(x)x=x;", Style));
+  EXPECT_EQ("`define X  \\\n"
+"  x = x;   \\\n"
+"  x = x;",
+format("`define X x=x;x=x;", Style));
+  // Macro definitions with invocations inside.
+  EXPECT_EQ("`define LIST   \\\n"
+"  `ENTRY   \\\n"
+"  `ENTRY",
+format("`define LIST \\\n"
+   "`ENTRY \\\n"
+   "`ENTRY",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST \\\n"
+   "`x = `x; \\\n"
+   "`x = `x;",
+   Style));
+  EXPECT_EQ("`define LIST   \\\n"
+"  `x = `x; \\\n"
+"  `x = `x;",
+format("`define LIST `x=`x;`x=`x;", Style));
+  // Macro invocations.
+  verifyFormat("`x = (`x1 + `x2 + x);");
+  // Lines starting with a normal macro invocation should be indented as a
+  // normal line.
+  EXPECT_EQ("if (x)\n"
+"  `x = `x;\n"
+"`timescale 1ns / 1ps",
+format("if (x)\n"
+   "`x = `x;\n"
+   "`timescale 1ns / 1ps",
+   Style));
+  EXPECT_EQ("if (x)\n"
+"`timescale 1ns / 1ps\n"
+"  `x = `x;",
+format("if (x)\n"
+   "`timescale 1ns / 1ps\n"
+   "`x = `x;",
+   Style));
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1780,8 +1780,23 @@
 break;
 
   TokenCount = Line->Tokens.size();
-  if (TokenCount == 1 ||
-  (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {
+  // Determine whether the line might be a single macro expansion.
+  // In Verilog macro expansions begin with a backtick.
+  {
+const UnwrappedLineNode *Tok = >Tokens.front(),
+*End = Tok + TokenCount;
+while (Tok != End && Tok->Tok->is(tok::comment))
+  ++Tok;
+if (Style.Language == FormatStyle::LK_Verilog) {
+  if (Tok != End && Tok->Tok->is(tok::hash))
+++Tok;
+  else
+break;
+}
+if (End - Tok != 1)
+  break;
+  }
+
 if (FormatTok->is(tok::colon) && !Line->MustBeDeclaration) {
   Line->Tokens.begin()->Tok->MustBreakBefore = true;
   parseLabel(!Style.IndentGotoLabels);
@@ -1805,7 +1820,6 @@
   PreviousToken->setFinalizedType(TT_FunctionLikeOrFreestandingMacro);
   addUnwrappedLine();
   return;
-}
   }
   break;
 }
@@