port-name-suffix: handle ref direction and all-underscore names - #2534
port-name-suffix: handle ref direction and all-underscore names#2534EylonKrause wants to merge 2 commits into
ref direction and all-underscore names#2534Conversation
PortNameSuffixRule crashed the linter on two valid inputs: - A `ref` port direction: IsSuffixCorrect did suffixes.at(direction) on a map keyed only by input/output/inout, so a `ref` port (allowed by the grammar) threw std::out_of_range, uncaught, aborting the linter. Look the direction up with find() and treat an unknown direction as correct (no violation), matching Violation() which already ignores non-input/output/inout. - An all-underscore name (e.g. "_"): absl::StrSplit with SkipEmpty yields an empty parts list, and the size()<2 branch did not return, so name_parts.back() dereferenced an empty vector (UB). Return after reporting the violation; this also drops a redundant duplicate Violation for no-underscore names. Adds regression tests: a `ref` port is accepted, and an all-underscore port reports a single suffix violation instead of crashing. Signed-off-by: Eylon Krause <eylon1909@gmail.com>
|
|
|
Disclosure: this contribution was authored with an AI coding assistant (Claude) and reviewed before submission. |
| // safely and treat an unknown direction as "correct" (no violation), | ||
| // consistent with Violation() which also ignores non-input/output/inout | ||
| // directions. Using std::map::at() here would throw on e.g. "ref". | ||
| const auto it = suffixes.find(direction); |
There was a problem hiding this comment.
We have the FindWithDefault() in verible/common/util/container-util.h
Maybe use that instead, not only for a more compact writing but also a more visible 'we're looking for something that might not be there but we have a fallback' intent.
return FindWithDefault(suffixes, direction, 1) == 1;
hzeller
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
LGTM, but I have one comment about using a map lookup function that might be nicer in this case.
|
Thanks! Done — switched to static const std::set<std::string_view> kNoConvention;
const std::set<std::string_view> &valid =
verible::container::FindWithDefault(suffixes, direction, kNoConvention);
return valid.empty() || valid.count(suffix) == 1; |
Address review feedback: look the direction up with verible::container::FindWithDefault and an empty-set fallback instead of a manual find()/end() check. Since the map holds a set of valid suffixes per direction, an unknown direction (e.g. `ref`) yields an empty set and is treated as correct, preserving the existing behavior. Signed-off-by: Eylon Krause <eylon1909@gmail.com>
d3a3deb to
324346b
Compare
Problem
PortNameSuffixRulecrashes the linter on two kinds of valid SystemVerilog:refport direction throwsstd::out_of_range(uncaught → linter abort)._) dereferences an empty vector (UB).Details
1.
refdirection —IsSuffixCorrectdoessuffixes.at(direction), wheresuffixesonly has keys{input, output, inout}. The grammar'sport_direction : dir | TK_refallowsref, so a port likemodule m(ref logic data_x); endmodulereachessuffixes.at("ref"), which throws.HandleSymbolruns without a surrounding try/catch, so the linter terminates. (The comment claiming the direction "is guaranteed" to be a known key is incorrect.)2. all-underscore name —
name_parts = absl::StrSplit(name, '_', absl::SkipEmpty()). For a name that is only underscores (_,__), every token is empty andSkipEmptydrops them all, leavingname_partsempty. Theif (name_parts.size() < 2)block reports a violation but does not return, soname_parts.back()on the next line dereferences an empty vector (UB).Fix
find()and treat an unknown direction (e.g.ref) as "correct" — no violation — matchingViolation(), which already ignores non-input/output/inout directions.returnafter thesize() < 2violation (a name with fewer than two underscore-delimited parts has no suffix to check). This also removes a redundant duplicateViolationinsert for no-underscore names.Tests
port-name-suffix-rule_test.cc: arefport is accepted (no violation, no crash); an all-underscore_port reports a single suffix violation instead of crashing.