MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
558 stars 122 forks source link

[fix][slang][nondegeneracy] Fix implication consequent nondegeneracy check #1023

Closed likeamahoney closed 2 weeks ago

likeamahoney commented 2 weeks ago

Fixes https://github.com/MikePopoloski/slang/issues/1022

The consequent of the implication should be ignored, since there are no restrictions on it in the standard.

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.68%. Comparing base (1d6eb64) to head (06da8b5).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023/graphs/tree.svg?width=650&height=150&src=pr&token=sS5JjK9091&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski)](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) ```diff @@ Coverage Diff @@ ## master #1023 +/- ## ======================================= Coverage 94.68% 94.68% ======================================= Files 191 191 Lines 47561 47563 +2 ======================================= + Hits 45031 45034 +3 + Misses 2530 2529 -1 ``` | [Files](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) | Coverage Δ | | |---|---|---| | [include/slang/ast/expressions/AssertionExpr.h](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?src=pr&el=tree&filepath=include%2Fslang%2Fast%2Fexpressions%2FAssertionExpr.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski#diff-aW5jbHVkZS9zbGFuZy9hc3QvZXhwcmVzc2lvbnMvQXNzZXJ0aW9uRXhwci5o) | `58.92% <ø> (ø)` | | | [source/ast/expressions/AssertionExpr.cpp](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?src=pr&el=tree&filepath=source%2Fast%2Fexpressions%2FAssertionExpr.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski#diff-c291cmNlL2FzdC9leHByZXNzaW9ucy9Bc3NlcnRpb25FeHByLmNwcA==) | `89.87% <100.00%> (+0.14%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Last update [1d6eb64...06da8b5](https://app.codecov.io/gh/MikePopoloski/slang/pull/1023?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski).
likeamahoney commented 2 weeks ago

Have no idea why it's segfaults on ThreadPoolTests. After looking on other PR's I suppose this is a common problem for current CI state.

likeamahoney commented 2 weeks ago

Have no idea why it's segfaults on ThreadPoolTests. After looking on other PR's I suppose this is a common problem for current CI state.

win64-release build issue

MikePopoloski commented 2 weeks ago

Looks like the Windows builds are broken only on GitHub due to this: https://github.com/actions/runner-images/issues/10004

MikePopoloski commented 2 weeks ago

I don't think this is the right solution. The rule "Any sequence that is used as a property shall be nondegenerate and shall not admit any empty match." should still apply here. The reason it's broken is that the checking for the "within" condition is just incorrect. I'll see about putting up an alternative fix later today.

MikePopoloski commented 2 weeks ago

Fixed in 2122d639817cca5234d377baa2e1b2aa8abf4592 instead