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

Add multiple slang-tidy checks and minor fixes in existing ones #1040

Merged
merged 11 commits into from
Jul 1, 2024

Conversation

JoelSole-Semidyn
Copy link
Contributor

@JoelSole-Semidyn JoelSole-Semidyn commented Jun 28, 2024

This PR adds some new slang-tidy checks, along with some minor fixes to existing ones, mainly aiming to throw the warnings in a more precise location. The new slang-tidy checks added to this PR are the following:

style checks

  • AlwaysCombNamed
    This check ensures that all always_comb blocks have a name

  • GenerateNamed
    Similar to the previous check, this ensures that all generate blocks are named, including for loops

  • NoDotVarInPortConnection
    A check analogous to NoDotStarInPortConnection, enforces that no '.var' is used in port connection, and '.var(var)' should be used instead

  • NoLegacyGenerate
    This check enforces that no generate block is used in the code, since it is an old system verilog syntax

synthesis checks

  • AlwaysFFAssignmentOutsideConditional
    Check that ensures that if there is a conditional block with reset, all the assignments must be either inside the if or else statements

  • UnusedSensitiveSignal
    This check verifies that every signal inside the sensitivity list of an 'always' or 'always_ff' block is used inside the block statement. The only exception is the clock signal

This new check enforces that each always_comb block is named
This new check enforces that each generate block is named.
This also is applied to the for block
This check ensures that no module instantiation uses the sintax '.var' in
the port connection, and ensures that '.var(var)' is used instead
Now, when '.*' is used with the NoDotStarInPortConnection check enabled,
the warning will be located where the '.*' character is, instead of the
location of the module that was being called.
Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.71%. Comparing base (c2c478c) to head (1108fb5).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1040   +/-   ##
=======================================
  Coverage   94.70%   94.71%           
=======================================
  Files         191      191           
  Lines       47655    47655           
=======================================
+ Hits        45133    45135    +2     
+ Misses       2522     2520    -2     

see 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2c478c...1108fb5. Read the comment docs.

@jcurtiss8086
Copy link

I have not reviewed all of the current Tidy checks, so my apologies if this is covered. NoForInsideGenerate seems like it would want an equivalent NoIfInsideGenerate as well.

Rather than having separate options to flag the use of for/if blocks inside generate statemnts, how about a different option NoLegacyGenerate which just flags any use of the keyword 'generate' at all? This is an option that Verible has for its lint rules.

This new check enforces that no generate block is used in the code,
since it is an old system verilog syntax
This new synthesis check ensures that if there is a conditional
block with reset, all the assignments must be either inside the
if or else statements.
This new check verifies that every signal inside the sensitivity list
of an 'always' or 'always_ff' block is used inside the block
statement. The only exception is the clock signal
* slang-tidy fix OnlyAssignedOnReset and RegisterHasNoReset:
This commit will ensure that both checks will no longer be ignoring the
configuration setting 'resetIsActiveHigh' and the possible negation of
the reset signal. Two test cases have also been added to consider this
bug.

* The error location of the warnings generated by the
OnlyAssignedOnReset and RegisterHasNoReset are now more precise and
target the variable instance where the error is found instead of the
variable declaration.
@JoelSole-Semidyn
Copy link
Contributor Author

You are totally right, so I just uploaded a new version with the check NoLegacyGenerate instead of NoForInsideGenerate

I have not reviewed all of the current Tidy checks, so my apologies if this is covered. NoForInsideGenerate seems like it would want an equivalent NoIfInsideGenerate as well.

Rather than having separate options to flag the use of for/if blocks inside generate statemnts, how about a different option NoLegacyGenerate which just flags any use of the keyword 'generate' at all? This is an option that Verible has for its lint rules.

@MikePopoloski MikePopoloski merged commit b7218d1 into MikePopoloski:master Jul 1, 2024
15 checks passed
JoelSole-Semidyn added a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 12, 2024
…dev/slang-tidy

* 'master' of https://github.com/MikePopoloski/slang:
  Update CHANGELOG.md
  [slang] Fix PLA task concatenation ascending order (MikePopoloski#1047)
  [slang] Fix PATHPULSE limit values (MikePopoloski#1048)
  Handle recursive parameter definitions via hierarchical reference
  Mark fmt lib headers as system headers to suppress warnings
  Bump dependency versions: fmt and pybind11
  [slang] Make string simple type (MikePopoloski#1049)
  Update README.md (MikePopoloski#1046)
  [slang][port] Fix bugs (MikePopoloski#1043)
  Use [[likely]] / [[unlikely]] replace macro SLANG_LIKELY/SLANG_UNLIKELY (MikePopoloski#1038)
  Add multiple slang-tidy checks and minor fixes in existing ones (MikePopoloski#1040)
  chore: update pre-commit hooks (MikePopoloski#1041)
JoelSole-Semidyn added a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 15, 2024
…dev/slang-tidy

* 'master' of https://github.com/MikePopoloski/slang:
  Fix a bug in the tidy-check UnusedSensitiveSignal (MikePopoloski#1056)
  Update CHANGELOG.md
  [slang] Fix PLA task concatenation ascending order (MikePopoloski#1047)
  [slang] Fix PATHPULSE limit values (MikePopoloski#1048)
  Handle recursive parameter definitions via hierarchical reference
  Mark fmt lib headers as system headers to suppress warnings
  Bump dependency versions: fmt and pybind11
  [slang] Make string simple type (MikePopoloski#1049)
  Update README.md (MikePopoloski#1046)
  [slang][port] Fix bugs (MikePopoloski#1043)
  Use [[likely]] / [[unlikely]] replace macro SLANG_LIKELY/SLANG_UNLIKELY (MikePopoloski#1038)
  Add multiple slang-tidy checks and minor fixes in existing ones (MikePopoloski#1040)
  chore: update pre-commit hooks (MikePopoloski#1041)
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.

None yet

3 participants