Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding regex configuration to the parameter-name-style rule #2213

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

sconwayaus
Copy link
Contributor

Adding regex configuration to the parameter-name-style rule, part of issue #2074

@sconwayaus sconwayaus force-pushed the parameter_name_style_regex branch 2 times, most recently from 43a5479 to d3a0a57 Compare July 9, 2024 11:37
Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make sure to be able to use the old configuration value with the named values.

configuration,
{{"localparam_style", SetNamedBits(&localparam_allowed_style_, choices)},
{"parameter_style", SetNamedBits(&parameter_allowed_style_, choices)}});
{{"localparam_style_regex", SetRegex(&localparam_style_regex_)},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't remove the old configuration names: if someone has used localparam_style and parameter_style in their configuration it will suddenly fail as it doesn't exist anymore.

Instead, we should have not only the new regex names, but also the old names, which then select a regex.
So someone can set localparam_style_regex, but they can also say localparam_style=ALL_CAPS, which could be considered a name for a regex; So if such value has been set, we then then can set the upper case regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a preference on how it should handle if both localparam_style and localparam_style_regex are set with valid values?

Maybe have ParameterNameStyleRule::Configure return an absl::InvalidArgumentError() noting the user should use one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, maybe ignore localparam_style_regex unless the user sets localparam_style to some value (say "regex") so the user has to explicitly enable that feature.

Just looking at the code again, localparam_style is an OR'd list of things, so when the rule is configured it would have OR multiple regex's together, potentially including the users regex, simple enough.

Anyway, let me know if you have a preference (or better solution).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, the OR-ing part, I didn't think of it: yes that should then assemble a (foo)|(bar) kindof regex.

But with OR-ing in place, we can assemble all the regex from localparam_style with the localparam_style_regex if both of them are given in the lint configuration.

Probably good to have a test for all these situations; for that, you could have public functions that return the const RE2 * for each regex

const RE2 *localparam_style_regex() const { return localparam_style_regex_.get(); }

, so that it is easy to write a test that verifies that the returned value is re != nullptr and the re->pattern() matches the expectation after having called Configuration().

verilog/analysis/checkers/parameter_name_style_rule.cc Outdated Show resolved Hide resolved
const std::initializer_list<LintTestCase> kTestCases = {
{"package a; parameter int FooBar = 1; endpackage"},
{"package a; parameter int ", {kToken, "FOO_BAR"}, " = 1; endpackage"},
{"module a; localparam int ", {kToken, "FooBar"}, " = 1; endmodule"},
{"module a; localparam int FOO_BAR = 1; endmodule"},
};
RunConfiguredLintTestCases<VerilogAnalyzer, ParameterNameStyleRule>(
kTestCases, "parameter_style:CamelCase;localparam_style:ALL_CAPS");
kTestCases,
"parameter_style_regex:([A-Z0-9]+[a-z0-9]*)+(_[0-9]+)?;localparam_"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example shows how hard it is to actually use if just the defaults would be needed; so making it possible to set the parameter_style:CamelCase will not only be backwards compatible, but also much more usable while still keeping the possibility open to actually use the regex.

@sconwayaus sconwayaus force-pushed the parameter_name_style_regex branch 4 times, most recently from 36e5bcb to 03ad538 Compare August 9, 2024 14:49
…. The rule now forms a single regex using the original configuration values and a user regex.
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.14085% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.92%. Comparing base (054f273) to head (23d76be).
Report is 62 commits behind head on master.

Files Patch % Lines
...log/analysis/checkers/parameter_name_style_rule.cc 89.55% 7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2213    +/-   ##
========================================
  Coverage   92.92%   92.92%            
========================================
  Files         359      360     +1     
  Lines       26740    26912   +172     
========================================
+ Hits        24848    25008   +160     
- Misses       1892     1904    +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sconwayaus
Copy link
Contributor Author

Ok, I think I've addressed all issues, with the regex configuration now extending the existing configuration in a non-breaking way.

@hzeller
Copy link
Collaborator

hzeller commented Sep 6, 2024

Oh, got distracted. Will do another review today.

@hzeller hzeller merged commit 2785d9b into chipsalliance:master Sep 17, 2024
34 checks passed
@hzeller
Copy link
Collaborator

hzeller commented Sep 17, 2024

Thanks! Sorry for the delay, sometimes life is just busy...

@sconwayaus
Copy link
Contributor Author

All good.
Cheers

@sconwayaus sconwayaus deleted the parameter_name_style_regex branch September 17, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants