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

[slang] Fix PATHPULSE limit values #1048

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

likeamahoney
Copy link
Contributor

In order to SystemVerilog LRM grammar it is legal to specialize only 1 value for both PATHPULSE$ reject and error limits.

According to 30.7.1 section:

pulse_control_specparam ::=
          PATHPULSE$ = ( reject_limit_value [ , error_limit_value ] )
       | PATHPULSE$specify_input_terminal_descriptor$specify_output_terminal_descriptor
= ( reject_limit_value [ , error_limit_value ] )

At the text below LRM says:

"The path (data=>q) is not explicitly defined in any of the PATHPULSE$ declarations; therefore, it acquires reject and error limit of 3, as defined by the last PATHPULSE$ declaration." for such example:

specify
...
    (data => q) = 10;
...
    specparam
    ...
        PATHPULSE$ = 3;
endspecify

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.71%. Comparing base (4221f2f) to head (b16cdc5).
Report is 4 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1048   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files         191      191           
  Lines       47656    47662    +6     
=======================================
+ Hits        45136    45142    +6     
  Misses       2520     2520           
Files Coverage Δ
source/parsing/Parser_members.cpp 98.00% <100.00%> (-0.01%) ⬇️

... and 2 files 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 4221f2f...b16cdc5. Read the comment docs.

Copy link
Owner

@MikePopoloski MikePopoloski left a comment

Choose a reason for hiding this comment

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

Huh, not sure why I added this but it looks like you're correct.

@@ -3227,7 +3227,7 @@ SpecparamDeclaratorSyntax& Parser::parseSpecparamDeclarator(SyntaxKind parentKin
if (parentKind != SyntaxKind::SpecifyBlock)
addDiag(diag::PulseControlSpecifyParent, name.range());
else if (!expr2)
addDiag(diag::PulseControlTwoValues, expr1.sourceRange()) << name.range();
expr2 = &expr1;
Copy link
Owner

Choose a reason for hiding this comment

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

This should not duplicate the expression here -- leaving it null seems fine. The parse tree must maintain the property that visiting the tokens in it via depth-first traversal recovers the original source file as typed by the user.

Copy link
Contributor Author

@likeamahoney likeamahoney Jul 11, 2024

Choose a reason for hiding this comment

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

fixed in last commit with simple code refactoring

@MikePopoloski MikePopoloski merged commit 6bd3d64 into MikePopoloski:master Jul 11, 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 pushed a commit to JoelSole-Semidyn/slang that referenced this pull request Jul 15, 2024
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

2 participants