-
Notifications
You must be signed in to change notification settings - Fork 614
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
[LTL] Added overloadings for AssertProperty #4037
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a simple test?
@sequencer commented on this pull request.
Can you add a simple test?
This is already covered by several existing tests in LTLSpec, which is why I didn't add one.
|
src/main/scala/chisel3/ltl/LTL.scala
Outdated
/** | ||
* Overloads the apply method to allow for Bool expressions to be used in AssertProperty. | ||
* This avoids having to import Sequence on the user side to get the implicit conversion. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's really a reason to mention overloading in the ScalaDoc, from the user's perspective this is just the API they should call (I would just basically copy-paste the ScalaDoc on the main apply function although it would be good to turn those bullets into actual Scaladoc (using @param
).
We also need a form that takes clock
and disable
. This is annoying since we can't have another apply with default arguments, but maybe just have 3, one taking the prop
, one taking the prop
and label
, and one taking all of the arguments.
To elaborate a little, places like
|
(cherry picked from commit 5cf2b40) # Conflicts: # src/main/scala/chisel3/ltl/LTL.scala
(cherry picked from commit 5cf2b40)
(cherry picked from commit 5cf2b40) Co-authored-by: Amelia Dobis <[email protected]>
This PR adds two overloadings for
AssertProperty
, which allow for it to be used onBool
s without importingSequence._
.Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.