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: Add suspicious-semicolon rule #2016

Merged

Conversation

IEncinas10
Copy link
Collaborator

Inspired by clang-tidy's similar check.

Things to note:

  • verilog/tools/lint/testdata/forbid_consecutive_null_statement.sv has been modified so it doesn't fail under this rule too.

I don't think this will flag any code that isn't wrong except the typical

while(myCoolFunction());

which I don't think is a particular common idiom in [System]Verilog. It can be fixed with an empty begin end. I could also make the check more specific (require that there isn't a function or task call inside the while condition (?)).

Maybe this could be enabled by default?

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

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

Comparison is base (e76f7fc) 92.95% compared to head (ebc9dc5) 92.95%.

Files Patch % Lines
...log/analysis/checkers/suspicious_semicolon_rule.cc 95.00% 1 Missing ⚠️

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2016   +/-   ##
=======================================
  Coverage   92.95%   92.95%           
=======================================
  Files         357      358    +1     
  Lines       26412    26432   +20     
=======================================
+ Hits        24550    24569   +19     
- Misses       1862     1863    +1     

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

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.

This is a very cool idea, thanks!
One idea for another test case and a fully qualified include, and we should be ready to go.

Inspired by clang-tidy's similar check.

Things to note: verilog/tools/lint/testdata/forbid_consecutive_null_statement.sv
has been modified so it doesn't fail under this rule too.
@IEncinas10 IEncinas10 force-pushed the bugprone-suspicious-semicolon branch from fccb08b to ebc9dc5 Compare December 20, 2023 19:48
@IEncinas10
Copy link
Collaborator Author

What do you think about enabling by default?

@hzeller
Copy link
Collaborator

hzeller commented Dec 21, 2023

I think considering it by default sounds like a good idea, but I like to separate out the feature itself (default off) and then a separate pull request that only switches the default. That allows for a nicer rollout experience.

@hzeller hzeller merged commit 4e8133d into chipsalliance:master Dec 21, 2023
30 of 31 checks passed
@IEncinas10 IEncinas10 deleted the bugprone-suspicious-semicolon branch January 1, 2024 15:11
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