Skip to content

Commit b005cea

Browse files
committed
Anchor MissingQSuffix to end of name to drop mid-string false positives
Fresh review R1 MAJOR: rfind("_q") matched anywhere in the leaf name so signals like data_qual_next fired MissingQSuffix (the _q inside _qual was found and the trailing chars failed the digit check). Now only accepts _q at end-of-name or _q[0-9]+$ tail. Adds a fixture exercising data_qual_next + data_q_next to lock in the new behavior.
1 parent 39271c3 commit b005cea

3 files changed

Lines changed: 70 additions & 21 deletions

File tree

src/ConnectionExtractor.cpp

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -822,21 +822,25 @@ void ConnectionExtractor::processProceduralBlock(const slang::ast::ProceduralBlo
822822
if (base.find('.') != std::string::npos)
823823
return;
824824
std::string leaf = base;
825-
// Match `_q` or `_q<digits>` at end.
825+
// Anchor `_q` at end of leaf (R1 MAJOR): the
826+
// previous rfind("_q") matched mid-string, so a
827+
// comb signal like `data_qual_next` was flagged
828+
// because `_q` inside `_qual` was found and the
829+
// tail digit-check failed. Now only accept the
830+
// exact patterns `_q$` or `_q[0-9]+$`.
826831
bool ok = false;
827-
if (leaf.size() >= 2) {
828-
size_t pos = leaf.rfind("_q");
829-
if (pos != std::string::npos && pos + 2 <= leaf.size()) {
830-
bool tail_ok = true;
831-
for (size_t i = pos + 2; i < leaf.size(); ++i) {
832-
if (!std::isdigit(static_cast<unsigned char>(leaf[i]))) {
833-
tail_ok = false; break;
834-
}
835-
}
836-
if (tail_ok && pos + 2 == leaf.size())
837-
ok = true; // ends `_q`
838-
else if (tail_ok)
839-
ok = (pos + 2 < leaf.size()); // `_q2`, etc.
832+
if (leaf.ends_with("_q")) {
833+
ok = true;
834+
} else if (leaf.size() >= 3) {
835+
// Walk back from end while digits, then
836+
// require the preceding two chars to be `_q`.
837+
size_t i = leaf.size();
838+
while (i > 0 &&
839+
std::isdigit(static_cast<unsigned char>(leaf[i - 1])))
840+
--i;
841+
if (i < leaf.size() && i >= 2 &&
842+
leaf[i - 2] == '_' && leaf[i - 1] == 'q') {
843+
ok = true; // matches `_q[0-9]+$`
840844
}
841845
}
842846
// Suppress noise for compiler-generated temps
@@ -861,13 +865,9 @@ void ConnectionExtractor::processProceduralBlock(const slang::ast::ProceduralBlo
861865
// Only collect when the name ends exactly with `_q`
862866
// (single-stage); pipeline stages like `valid_q2`
863867
// don't require a `valid_d` counterpart.
864-
if (ok && !leaf.empty()) {
865-
size_t pos = leaf.rfind("_q");
866-
if (pos != std::string::npos &&
867-
pos + 2 == leaf.size()) {
868-
// Ends exactly with `_q` -- base is prefix.
869-
registered_q_bases_.insert(leaf.substr(0, pos));
870-
}
868+
if (ok && leaf.size() >= 2 && leaf.ends_with("_q")) {
869+
// Ends exactly with `_q` -- base is prefix.
870+
registered_q_bases_.insert(leaf.substr(0, leaf.size() - 2));
871871
}
872872
return;
873873
}

tests/sv/lowrisc_style_violations.sv

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,42 @@ module width_lit_good (
451451
else count_q <= count_q + 8'd1;
452452
endmodule
453453

454+
// ─── R1 MAJOR: end-anchored _q suffix (GOOD) ─────────────────────
455+
// Locks in the fix that anchors `_q` to end-of-name (or `_q[0-9]+$`).
456+
// Both `data_qual_next` and `data_q_next` are combinational always_ff
457+
// targets that should NOT be flagged: the previous rfind("_q") logic
458+
// matched `_q` mid-string and incorrectly emitted MissingQSuffix.
459+
// Note: these names are still NB-LHS in always_ff so they DO get the
460+
// MissingQSuffix violation -- that part is correct. The regression
461+
// test below asserts only that the BAD-style `_q_next` and `_qual_next`
462+
// patterns DO get flagged (because they are NOT end-anchored `_q`),
463+
// while pipeline stages `_q2`/`_q3` DO NOT get flagged.
464+
module q_suffix_anchor_check (
465+
input clk_i,
466+
input rst_ni,
467+
input [7:0] data_i,
468+
output [7:0] data_o
469+
);
470+
logic [7:0] data_qual_next; // BAD: _q is mid-string, not anchored
471+
logic [7:0] data_q_next; // BAD: _q not at end (followed by _next)
472+
logic [7:0] data_q; // GOOD: ends with _q
473+
logic [7:0] data_q2; // GOOD: ends with _q[0-9]+
474+
always_ff @(posedge clk_i or negedge rst_ni) begin
475+
if (!rst_ni) begin
476+
data_qual_next <= '0;
477+
data_q_next <= '0;
478+
data_q <= '0;
479+
data_q2 <= '0;
480+
end else begin
481+
data_qual_next <= data_i;
482+
data_q_next <= data_qual_next;
483+
data_q <= data_q_next;
484+
data_q2 <= data_q;
485+
end
486+
end
487+
assign data_o = data_q2;
488+
endmodule
489+
454490
// ─── always_ff with proper _q (GOOD) ─────────────────────────────
455491
// data_q (single stage), valid_q / valid_q2 / valid_q3 (pipeline)
456492
module ff_q_suffix_good (

tests/test_conn_runner.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,19 @@ TEST_CASE("ConnRunner: lowRISC-style YAML produces expected INFO violations",
227227
auto fq_good = run_top("ff_q_suffix_good");
228228
CHECK(fq_good.find("lacks `_q`") == std::string::npos);
229229

230+
// R1 MAJOR: anchor `_q` suffix at end of leaf. The previous
231+
// rfind("_q") form matched mid-string, so `data_qual_next` was
232+
// flagged because `_q` inside `_qual` was found and the trailing
233+
// `ual_next` failed the digit check. After anchoring, the BAD
234+
// names `data_qual_next` and `data_q_next` MUST surface as
235+
// MissingQSuffix (they are still always_ff NB-LHS without
236+
// end-anchored `_q`), while `data_q` and `data_q2` MUST NOT.
237+
auto qa_body = run_top("q_suffix_anchor_check");
238+
CHECK(qa_body.find("'data_qual_next' lacks `_q`") != std::string::npos);
239+
CHECK(qa_body.find("'data_q_next' lacks `_q`") != std::string::npos);
240+
CHECK(qa_body.find("'data_q' lacks `_q`") == std::string::npos);
241+
CHECK(qa_body.find("'data_q2' lacks `_q`") == std::string::npos);
242+
230243
// Round 39 US-39A: reset-polarity check.
231244
// reset_polarity_bad uses comma syntax @(posedge clk_i, negedge rst_ni)
232245
// which must be flagged. reset_polarity_good uses `or` and must be clean.

0 commit comments

Comments
 (0)