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

linter: make constraint-name-style configurable #2017

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IEncinas10
Copy link
Collaborator

@IEncinas10 IEncinas10 commented Sep 23, 2023

Up until now, constraint-name-style rule required constraint names to end in _c. This makes it configurable, so that users can require constraint names to start with c_ instead.

I'm open to sugestions about the configuration parameter's name, it doesn't convince me too much but I can't think of a better option. We could have check_suffix and check_prefix but as it doesn't make sense for them to be on at the same time I don't like it too much either.


Edit: forgot to handle this, leaving it here so I don't forget

  • Handle invalid regex provided by user

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a4d61b1) 92.95% compared to head (846710b) 92.95%.

Files Patch % Lines
...og/analysis/checkers/constraint_name_style_rule.cc 94.11% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2017      +/-   ##
==========================================
- Coverage   92.95%   92.95%   -0.01%     
==========================================
  Files         358      359       +1     
  Lines       26446    26458      +12     
==========================================
+ Hits        24583    24594      +11     
- Misses       1863     1864       +1     

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

@hzeller
Copy link
Collaborator

hzeller commented Dec 19, 2023

(Sorry for the long delay, I was pretty busy with other things...)

Maybe we can provide a regular expression, e.g. configuration parameter name pattern, and then the user can decide. And then we can set the default to "([a-z0-9]+_)+c" or so to enforce snake-case with trailing-_c' (we can keep the regex simple and don't have to worry about the beginning starting with a digit as that already has been taken care of by the lexer/parser)

Using regular expressions is probably also a good choice to configure some of the other rules - they are
We use the re2 library successfully in other parts of the project e.g. in ./common/tools/jcxxgen.cc and ./verilog/tools/ls/autoexpand.cc.

@IEncinas10
Copy link
Collaborator Author

No worries, first things first!

I remember seeing discussion about regular expressions for some other rules in the past but thought the support wasn't there yet. I'll give this a look and definitely change it. If it ends up being as easy at it sounds I'll make an issue to eventually update every rule that could make use of this feature.

@hzeller
Copy link
Collaborator

hzeller commented Dec 21, 2023

At the time we had the disccussions about regular expressions, the choice was std::regex, but that is generally a poor choice as it is slow. Since then, RE2 had been added to the dependencies, and that is pretty performant.

@IEncinas10 IEncinas10 force-pushed the configurable-constraint-name-style-rule branch from 5e9b7da to bbccb82 Compare January 13, 2024 16:52
static const Matcher& ConstraintMatcher() {
absl::Status ConstraintNameStyleRule::Configure(
absl::string_view configuration) {
std::string pattern = kSuffix;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is redundant but the rationale is the following:

If at some point we add other parameters to the rule, but pattern is not provided, the ParseNameValues wouldn't set any value to pattern, so we would then need to set a default value.

Avoid future problems by doing it already?

violations_.insert(LintViolation(*identifier_token, kMessage, context));
if (!RE2::FullMatch(constraint_name, *regex)) {
violations_.insert(
LintViolation(*identifier_token, FormatReason(), context));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't find many resources on RE2. Can it somehow help us provide a better linting message than just "follow the regex"? Perhaps provide an autofix?

@IEncinas10 IEncinas10 force-pushed the configurable-constraint-name-style-rule branch from bbccb82 to 7332d1b Compare January 14, 2024 09:44
Up until now, constraint-name-style rule required constraint names to
end in '_c'. This makes it configurable, so that users can provide a
regular expression to specify what name style they want.

The default behaviour is still the same.
@IEncinas10 IEncinas10 force-pushed the configurable-constraint-name-style-rule branch from 7332d1b to 846710b Compare January 14, 2024 10:24
@IEncinas10 IEncinas10 requested a review from hzeller March 29, 2024 13:25
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