-
Notifications
You must be signed in to change notification settings - Fork 586
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
Make isView safe outside of Builder context #4228
Conversation
@@ -273,7 +273,7 @@ package object dataview { | |||
} | |||
|
|||
// Safe for all Data | |||
private[chisel3] def isView(d: Data): Boolean = d._parent.contains(ViewParent) | |||
private[chisel3] def isView(d: Data): Boolean = d._parent.exists(_ == ViewParent) |
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.
This looks equivalent but because contains
takes its argument by value, even if _parent
is None
, ViewParent
must be initialized (it's a lazy val) which requires a Chisel context (because it's a Module)... yay side effects 🤮.
@@ -275,6 +275,11 @@ trait ShiftRightWidthBehavior { self: ChiselRunners => | |||
|
|||
class UIntOpsSpec extends ChiselPropSpec with Matchers with Utils with ShiftRightWidthBehavior { | |||
|
|||
// This is intentionally a val outside of any ScalaTest constructs to check that it is legal | |||
// to create a literal outside of a Chisel context and *before* and Chisel contexts have been created |
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.
And-> any
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.
Not sure why the difference matters but looks good!
(cherry picked from commit 219a237)
(cherry picked from commit 219a237)
(cherry picked from commit 219a237)
Contemplating views in naming (#4222) exposed a longstanding bug that I patched out recently in #4147. Basically
isView
does not work outside of Builder contexts. Now it is safe.We do have lots of tests for things like literals outside of Builder contexts, but they didn't trigger this because
ViewParent
is alazy val
and thus if any Chisel elaboration has occurred on the same thread, then the problem doesn't manifest. Thus this "test" looks very strange, but by virtue of defining a literal assigned to a val first thing in a ScalaTest spec, it ensures no Chisel elaboration has run before when this line executes, and the naming plugin + #4222 ensures that this value is checked if it's a view.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
.