Skip to content

Commit

Permalink
Merge pull request #2190 from sconwayaus/package_localparam
Browse files Browse the repository at this point in the history 
[proper-parameter-declaration] parameter vs localparam in packages
  • Loading branch information
@hzeller
hzeller authored Jun 10, 2024
2 parents f273154 + d33c0a9 commit 48a03f3
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 32 deletions.
2 changes: 2 additions & 0 deletions verilog/analysis/checkers/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1745,6 +1745,7 @@ cc_library(
"//common/analysis:syntax-tree-lint-rule",
"//common/analysis/matcher",
"//common/analysis/matcher:bound-symbol-manager",
"//common/text:config-utils",
"//common/text:symbol",
"//common/text:syntax-tree-context",
"//verilog/CST:context-functions",
Expand All @@ -1753,6 +1754,7 @@ cc_library(
"//verilog/analysis:descriptions",
"//verilog/analysis:lint-rule-registry",
"//verilog/parser:verilog-token-enum",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings:string_view",
],
alwayslink = 1,
Expand Down
76 changes: 66 additions & 10 deletions verilog/analysis/checkers/proper_parameter_declaration_rule.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@

#include <set>

#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "common/analysis/lint_rule_status.h"
#include "common/analysis/matcher/bound_symbol_manager.h"
#include "common/analysis/matcher/matcher.h"
#include "common/text/config_utils.h"
#include "common/text/symbol.h"
#include "common/text/syntax_tree_context.h"
#include "verilog/CST/context_functions.h"
Expand All @@ -40,25 +42,66 @@ using verible::matcher::Matcher;
// Register ProperParameterDeclarationRule
VERILOG_REGISTER_LINT_RULE(ProperParameterDeclarationRule);

static constexpr absl::string_view kParameterMessage =
"\'parameter\' declarations should only be within packages or in the "
"formal parameter list of modules/classes.";
static constexpr absl::string_view kLocalParamMessage =
"\'localparam\' declarations should only be within modules\' or classes\' "
static constexpr absl::string_view kParameterNotInPackageMessage =
"\'parameter\' declarations should only be in the formal parameter list of "
"modules/classes.";
static constexpr absl::string_view kParameterAllowPackageMessage =
"\'parameter\' declarations should only be in the formal parameter list of "
"modules and classes or in package definition bodies.";
static constexpr absl::string_view kLocalParamNotInPackageMessage =
"\'localparam\' declarations should only be within modules or class "
"definition bodies.";
static constexpr absl::string_view kLocalParamAllowPackageMessage =
"\'localparam\' declarations should only be within modules, packages or "
"class definition bodies.";

static absl::string_view kParameterMessage = kParameterNotInPackageMessage;
static absl::string_view kLocalParamMessage = kLocalParamAllowPackageMessage;

const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() {
static const LintRuleDescriptor d{
.name = "proper-parameter-declaration",
.topic = "constants",
.desc =
"Checks that every `parameter` declaration is inside a "
"package or in the formal parameter list of modules/classes and "
"every `localparam` declaration is inside a module or class.",
};
"formal parameter list of modules/classes and "
"every `localparam` declaration is inside a module, class or "
"package.",
.param = {
{
.name = "package_allow_parameter",
.default_value = "true",
.description = "Allow parameters in packages (treated as a "
"synonym for localparam).",
},
{
.name = "package_allow_localparam",
.default_value = "false",
.description = "Allow localparams in packages.",
},
}};
return d;
}

absl::Status ProperParameterDeclarationRule::Configure(
absl::string_view configuration) {
using verible::config::SetBool;
auto status = verible::ParseNameValues(
configuration,
{
{"package_allow_parameter", SetBool(&package_allow_parameter_)},
{"package_allow_localparam", SetBool(&package_allow_localparam_)},
});

// Change the message slightly
kParameterMessage = package_allow_parameter_ ? kParameterAllowPackageMessage
: kParameterNotInPackageMessage;
kLocalParamMessage = package_allow_localparam_
? kLocalParamAllowPackageMessage
: kLocalParamNotInPackageMessage;
return status;
}

static const Matcher &ParamDeclMatcher() {
static const Matcher matcher(NodekParamDeclaration());
return matcher;
Expand All @@ -79,11 +122,24 @@ void ProperParameterDeclarationRule::HandleSymbol(
} else if (ContextIsInsideModule(context) &&
!ContextIsInsideFormalParameterList(context)) {
violations_.insert(LintViolation(symbol, kParameterMessage, context));
} else if (ContextIsInsidePackage(context)) {
if (!package_allow_parameter_) {
violations_.insert(LintViolation(symbol, kParameterMessage, context));
}
}
} else if (param_decl_token == TK_localparam) {
// If the context is not inside a class or module, report violation.
// Raise violation if the context is not inside a class, package or
// module, report violation.
if (!ContextIsInsideClass(context) && !ContextIsInsideModule(context)) {
violations_.insert(LintViolation(symbol, kLocalParamMessage, context));
if (!ContextIsInsidePackage(context)) {
violations_.insert(
LintViolation(symbol, kLocalParamMessage, context));
} else {
if (!package_allow_localparam_) {
violations_.insert(
LintViolation(symbol, kLocalParamMessage, context));
}
}
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions verilog/analysis/checkers/proper_parameter_declaration_rule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include <set>

#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "common/analysis/lint_rule_status.h"
#include "common/analysis/syntax_tree_lint_rule.h"
#include "common/text/symbol.h"
Expand All @@ -40,8 +42,13 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule {

verible::LintRuleStatus Report() const final;

absl::Status Configure(absl::string_view configuration) final;

private:
std::set<verible::LintViolation> violations_;

bool package_allow_parameter_ = true;
bool package_allow_localparam_ = false;
};

} // namespace analysis
Expand Down
133 changes: 111 additions & 22 deletions verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace analysis {
namespace {

using verible::LintTestCase;
using verible::RunConfiguredLintTestCases;
using verible::RunLintTestCases;

// Tests that ProperParameterDeclarationRule does not report a violation when
Expand All @@ -41,20 +42,28 @@ TEST(ProperParameterDeclarationRuleTest, BasicTests) {
RunLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(kTestCases);
}

// Tests that the expected number of parameter usage violations are found.
TEST(ProperParameterDeclarationRuleTest, ParameterTests) {
// Tests rejection of package parameters and allow package localparams
TEST(ProperParameterDeclarationRuleTest, RejectPackageParameters) {
const std::initializer_list<LintTestCase> kTestCases = {
{"parameter int Foo = 1;"},
{"package foo; parameter int Bar = 1; endpackage"},
{"package foo; parameter int Bar = 1; parameter int Bar2 = 2; "
{"package foo; ",
{TK_parameter, "parameter"},
" int Bar = 1; endpackage"},
{"package foo; ",
{TK_parameter, "parameter"},
" int Bar = 1; ",
{TK_parameter, "parameter"},
" int Bar2 = 2; "
"endpackage"},
{"module foo #(parameter int Bar = 1); endmodule"},
{"module foo #(int Bar = 1); endmodule"},
{"class foo #(parameter int Bar = 1); endclass"},
{"module foo #(parameter type Foo); endmodule"},
{"module foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endmodule"},
{"class foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endclass"},
{"package foo; class bar; endclass parameter int HelloWorld = 1; "
{"package foo; class bar; endclass ",
{TK_parameter, "parameter"},
" int HelloWorld = 1; "
"endpackage"},
{"package foo; class bar; ",
{TK_parameter, "parameter"},
Expand All @@ -69,8 +78,20 @@ TEST(ProperParameterDeclarationRuleTest, ParameterTests) {
{"module foo #(parameter type Bar); ",
{TK_parameter, "parameter"},
" type Bar2; endmodule"},
{"module foo #(parameter type Bar);"
"module innerFoo #("
"parameter type innerBar,",
"localparam int j = 2)();",
{TK_parameter, "parameter"},
" int i = 1;"
"localparam int j = 2;"
"endmodule "
"endmodule"},
};
RunLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(kTestCases);

RunConfiguredLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(
kTestCases,
"package_allow_parameter:false;package_allow_localparam:true");
}

// Tests that the expected number of localparam usage violations are found.
Expand All @@ -83,18 +104,17 @@ TEST(ProperParameterDeclarationRuleTest, LocalParamTests) {
{"module foo #(localparam int Bar = 1); endmodule"},
{"module foo #(localparam type Bar); endmodule"},
{"class foo #(localparam int Bar = 1); endclass"},
{{TK_localparam, "localparam"}, " int Bar = 1;"},
{"package foo; ",
{TK_localparam, "localparam"},
" int Bar = 1; endpackage"},
{"package foo; class bar; endclass ",
{TK_localparam, "localparam"},
" int HelloWorld = 1; "
{{TK_localparam, "localparam"},
" int Bar = 1;"}, // localparam defined outside a module or package
{"package foo; localparam int Bar = 1; endpackage"},
{"package foo; class bar; endclass localparam int HelloWorld = 1; "
"endpackage"},
{"package foo; class bar; localparam int HelloWorld = 1; endclass "
"endpackage"},
};
RunLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(kTestCases);
RunConfiguredLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(
kTestCases,
"package_allow_parameter:false;package_allow_localparam:true");
}

// Tests that the expected number of localparam and parameter usage violations
Expand All @@ -104,9 +124,10 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) {
{"parameter int Foo = 1; ",
{TK_localparam, "localparam"},
" int Bar = 1;"},
{"package foo; parameter int Bar = 1; ",
{TK_localparam, "localparam"},
" int Bar2 = 2; "
{"package foo; ",
{TK_parameter, "parameter"},
" int Bar = 1; ",
"localparam int Bar2 = 2; "
"endpackage"},
{"module foo #(parameter int Bar = 1); localparam int Bar2 = 2; "
"endmodule"},
Expand All @@ -121,17 +142,85 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) {
{"class foo; ",
{TK_parameter, "parameter"},
" int Bar = 1; localparam int Bar2 = 2; endclass"},
{"package foo; class bar; localparam int Bar2 = 2; endclass parameter "
"int HelloWorld = 1; "
{"package foo; class bar; localparam int Bar2 = 2; endclass ",
{TK_parameter, "parameter"},
" int HelloWorld = 1; "
"endpackage"},
{"package foo; ",
{TK_localparam, "localparam"},
" int Bar2 = 2; class bar; endclass parameter "
"int HelloWorld = 1; "
"localparam",
" int Bar2 = 2; class bar; endclass ",
{TK_parameter, "parameter"},
" int HelloWorld = 1; "
"endpackage"},
{"parameter int Foo = 1; module bar; localparam int Bar2 = 2; "
"endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(
kTestCases,
"package_allow_parameter:false;package_allow_localparam:true");
}

// package parameters allowed, package localparam's are rejected
TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) {
const std::initializer_list<LintTestCase> kTestCases = {
{"parameter int Foo = 1;"},
{"package foo; parameter int Bar = 1; endpackage"},
{"package foo; ",
{TK_localparam, "localparam"},
" int Bar = 1; endpackage"},
{"package foo; parameter int Bar = 1; "
"parameter int Bar2 = 2; "
"endpackage"},
{"module foo #(parameter int Bar = 1); endmodule"},
{"module foo #(int Bar = 1); endmodule"},
{"class foo #(parameter int Bar = 1); endclass"},
{"module foo #(parameter type Foo); endmodule"},
{"module foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endmodule"},
{"class foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endclass"},
{"package foo; class bar; endclass ",
"parameter int HelloWorld = 1; "
"endpackage"},
{"package foo; class bar; ",
{TK_parameter, "parameter"},
" int HelloWorld = 1; endclass "
"endpackage"},
{"module foo #(parameter int Bar = 1); ",
{TK_parameter, "parameter"},
" int HelloWorld = 1; "
"endmodule"},
{"module foo #(parameter type Foo, parameter int Bar = 1); "
"endmodule"},
{"module foo #(parameter type Bar); ",
{TK_parameter, "parameter"},
" type Bar2; endmodule"},
{"module foo #(parameter type Bar);"
"module innerFoo #("
"parameter type innerBar,",
"localparam int j = 2)();",
{TK_parameter, "parameter"},
" int i = 1;"
"localparam int j = 2;"
"endmodule "
"endmodule"},
{"module foo; localparam int Bar = 1; endmodule"},
{"class foo; localparam int Bar = 1; endclass"},
{"module foo; localparam int Bar = 1; localparam int Bar2 = 2; "
"endmodule"},
{"module foo #(localparam int Bar = 1); endmodule"},
{"module foo #(localparam type Bar); endmodule"},
{"class foo #(localparam int Bar = 1); endclass"},
{{TK_localparam, "localparam"}, " int Bar = 1;"},
{"package foo; ",
{TK_localparam, "localparam"},
" int Bar = 1; endpackage"},
{"package foo; class bar; endclass ",
{TK_localparam, "localparam"},
" int HelloWorld = 1; "
"endpackage"},
{"package foo; class bar; localparam int HelloWorld = 1; endclass "
"endpackage"},
};

RunLintTestCases<VerilogAnalyzer, ProperParameterDeclarationRule>(kTestCases);
}

Expand Down

0 comments on commit 48a03f3

Please sign in to comment.