[clang] [clang-format] Don't split "DPI"/"DPI-C" in Verilog imports (PR #66951)
Arthur Eubanks via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 21 09:55:49 PDT 2023
https://github.com/aeubanks updated https://github.com/llvm/llvm-project/pull/66951
>From a956ea32c145d84ee771c985ceb3becbd03f4022 Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Wed, 20 Sep 2023 13:42:20 -0700
Subject: [PATCH 1/4] [clang-format] Don't split "DPI"/"DPI-C" in imports
The spec doesn't allow splitting these strings and we're seeing compile issues with splitting it.
String splitting was enabled for Verilog in https://reviews.llvm.org/D154093.
---
clang/lib/Format/ContinuationIndenter.cpp | 8 ++++++++
clang/unittests/Format/FormatTestVerilog.cpp | 6 ++++++
2 files changed, 14 insertions(+)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index deb3e554fdc124b..0bdf339d8df5827 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -2270,7 +2270,15 @@ ContinuationIndenter::createBreakableToken(const FormatToken &Current,
if (State.Stack.back().IsInsideObjCArrayLiteral)
return nullptr;
+ // The "DPI" (or "DPI-C") in SystemVerilog direct programming interface
+ // imports cannot be split, e.g.
+ // `import "DPI" function foo();`
+ // FIXME: We should see if this is an import statement instead of hardcoding
+ // "DPI"/"DPI-C".
StringRef Text = Current.TokenText;
+ if (Style.isVerilog() && (Text == "\"DPI\"" || Text == "\"DPI-C\""))
+ return nullptr;
+
// We need this to address the case where there is an unbreakable tail only
// if certain other formatting decisions have been taken. The
// UnbreakableTailLength of Current is an overapproximation in that case and
diff --git a/clang/unittests/Format/FormatTestVerilog.cpp b/clang/unittests/Format/FormatTestVerilog.cpp
index 945e06143ccc3f1..56a8d19a31e919c 100644
--- a/clang/unittests/Format/FormatTestVerilog.cpp
+++ b/clang/unittests/Format/FormatTestVerilog.cpp
@@ -1253,6 +1253,12 @@ TEST_F(FormatTestVerilog, StringLiteral) {
"xxxx"});)",
R"(x({"xxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxx ", "xxxx"});)",
getStyleWithColumns(getDefaultStyle(), 23));
+ // "DPI"/"DPI-C" in imports cannot be split.
+ verifyFormat(R"(import
+ "DPI-C" function t foo
+ ();)",
+ R"(import "DPI-C" function t foo();)",
+ getStyleWithColumns(getDefaultStyle(), 23));
// These kinds of strings don't exist in Verilog.
verifyNoCrash(R"(x(@"xxxxxxxxxxxxxxxx xxxx");)",
getStyleWithColumns(getDefaultStyle(), 23));
>From 23d1b3c175b2b0d057014766cb558e9c6cfa67b4 Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Wed, 20 Sep 2023 23:08:42 -0700
Subject: [PATCH 2/4] Check if this is an import statement instead
---
clang/lib/Format/ContinuationIndenter.cpp | 15 +++++++++------
clang/unittests/Format/FormatTestVerilog.cpp | 2 +-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 0bdf339d8df5827..e279b42f1607345 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -2270,14 +2270,17 @@ ContinuationIndenter::createBreakableToken(const FormatToken &Current,
if (State.Stack.back().IsInsideObjCArrayLiteral)
return nullptr;
- // The "DPI" (or "DPI-C") in SystemVerilog direct programming interface
- // imports cannot be split, e.g.
+ // The "DPI"/"DPI-C" in SystemVerilog direct programming interface imports
+ // cannot be split, e.g.
// `import "DPI" function foo();`
- // FIXME: We should see if this is an import statement instead of hardcoding
- // "DPI"/"DPI-C".
StringRef Text = Current.TokenText;
- if (Style.isVerilog() && (Text == "\"DPI\"" || Text == "\"DPI-C\""))
- return nullptr;
+ if (Style.isVerilog()) {
+ const FormatToken *Prev = Current.getPreviousNonComment();
+ if (Prev && Prev == State.Line->getFirstNonComment() &&
+ Prev->TokenText == "import") {
+ return nullptr;
+ }
+ }
// We need this to address the case where there is an unbreakable tail only
// if certain other formatting decisions have been taken. The
diff --git a/clang/unittests/Format/FormatTestVerilog.cpp b/clang/unittests/Format/FormatTestVerilog.cpp
index 56a8d19a31e919c..c313046af23144a 100644
--- a/clang/unittests/Format/FormatTestVerilog.cpp
+++ b/clang/unittests/Format/FormatTestVerilog.cpp
@@ -1253,7 +1253,7 @@ TEST_F(FormatTestVerilog, StringLiteral) {
"xxxx"});)",
R"(x({"xxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxx ", "xxxx"});)",
getStyleWithColumns(getDefaultStyle(), 23));
- // "DPI"/"DPI-C" in imports cannot be split.
+ // import "DPI"/"DPI-C" cannot be split.
verifyFormat(R"(import
"DPI-C" function t foo
();)",
>From b9548587b2928ade3875a75abea891a6c8e666a1 Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Thu, 21 Sep 2023 09:53:55 -0700
Subject: [PATCH 3/4] Check kf prev token is import/export keyword
---
clang/lib/Format/ContinuationIndenter.cpp | 11 ++++-------
clang/unittests/Format/FormatTestVerilog.cpp | 9 ++++++---
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index e279b42f1607345..21a0efe23e72580 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -2273,14 +2273,11 @@ ContinuationIndenter::createBreakableToken(const FormatToken &Current,
// The "DPI"/"DPI-C" in SystemVerilog direct programming interface imports
// cannot be split, e.g.
// `import "DPI" function foo();`
- StringRef Text = Current.TokenText;
- if (Style.isVerilog()) {
- const FormatToken *Prev = Current.getPreviousNonComment();
- if (Prev && Prev == State.Line->getFirstNonComment() &&
- Prev->TokenText == "import") {
- return nullptr;
- }
+ if (Style.isVerilog() && Current.Previous &&
+ Current.Previous->isOneOf(tok::kw_export, Keywords.kw_import)) {
+ return nullptr;
}
+ StringRef Text = Current.TokenText;
// We need this to address the case where there is an unbreakable tail only
// if certain other formatting decisions have been taken. The
diff --git a/clang/unittests/Format/FormatTestVerilog.cpp b/clang/unittests/Format/FormatTestVerilog.cpp
index c313046af23144a..4c0a065daadd9ad 100644
--- a/clang/unittests/Format/FormatTestVerilog.cpp
+++ b/clang/unittests/Format/FormatTestVerilog.cpp
@@ -1253,11 +1253,14 @@ TEST_F(FormatTestVerilog, StringLiteral) {
"xxxx"});)",
R"(x({"xxxxxxxxxxxxxxxx xxxxxxxxxxxxxxxx ", "xxxx"});)",
getStyleWithColumns(getDefaultStyle(), 23));
- // import "DPI"/"DPI-C" cannot be split.
+ // import/export "DPI"/"DPI-C" cannot be split.
verifyFormat(R"(import
- "DPI-C" function t foo
+ "DPI-C" function void foo
();)",
- R"(import "DPI-C" function t foo();)",
+ R"(import "DPI-C" function void foo();)",
+ getStyleWithColumns(getDefaultStyle(), 23));
+ verifyFormat(R"(export "DPI-C" function foo;)",
+ R"(export "DPI-C" function foo;)",
getStyleWithColumns(getDefaultStyle(), 23));
// These kinds of strings don't exist in Verilog.
verifyNoCrash(R"(x(@"xxxxxxxxxxxxxxxx xxxx");)",
>From c78c547edc3fb4ace9d6e758a40aef16701283f9 Mon Sep 17 00:00:00 2001
From: Arthur Eubanks <aeubanks at google.com>
Date: Thu, 21 Sep 2023 09:55:33 -0700
Subject: [PATCH 4/4] Update comment
---
clang/lib/Format/ContinuationIndenter.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 21a0efe23e72580..68103c45b0b9463 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -2270,9 +2270,10 @@ ContinuationIndenter::createBreakableToken(const FormatToken &Current,
if (State.Stack.back().IsInsideObjCArrayLiteral)
return nullptr;
- // The "DPI"/"DPI-C" in SystemVerilog direct programming interface imports
- // cannot be split, e.g.
+ // The "DPI"/"DPI-C" in SystemVerilog direct programming interface
+ // imports/exports cannot be split, e.g.
// `import "DPI" function foo();`
+ // FIXME: make this use same infra as C++ import checks
if (Style.isVerilog() && Current.Previous &&
Current.Previous->isOneOf(tok::kw_export, Keywords.kw_import)) {
return nullptr;
More information about the cfe-commits
mailing list