-
Notifications
You must be signed in to change notification settings - Fork 597
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
Require AutoCloneType #2945
Require AutoCloneType #2945
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.
This is close but I think there was a slight miscommunication on the idea--it's not that users should be required to mixin AutoCloneType
, it's that AutoCloneType
applies to all Records automatically and manually implementing cloneType
is now banned.
plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Outdated
Show resolved
Hide resolved
src/test/scala/chiselTests/CompatibilityInteroperabilitySpec.scala
Outdated
Show resolved
Hide resolved
plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala
Outdated
Show resolved
Hide resolved
17524f3
to
2014b90
Compare
docs/src/cookbooks/cookbook.md
Outdated
class CustomBundleBroken(elts: (String, Data)*) extends Record { | ||
val elements = ListMap(elts.map { | ||
case (field, elt) => | ||
field -> DataMirror.internal.chiselTypeClone(elt) |
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.
is this what we want vs chiselTypeOf(...) ? I wish we could just settle on one API here.
In other words, I don't want to actively encourage some weird pattern that people should never be doing in the first place.
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 believe this is the right one, I'm not sure if chiselTypeOf clones the type
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.
chiselTypeOf clones the type, it's the same as chiselTypeClone except it also checks that the type is bound as hardware. In this case, the elts will not be bound hardware so we need chiselTypeClone.
I agree we should settle on one API but they have different uses and figuring out how to not need both uses is subtle and beyond this PR.
31b89a5
to
60e9fcc
Compare
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.
LGTM! I have a few very nitty suggestions but I will just merge them. Thanks!
Contributor Checklist
docs/src
?Type of Improvement
API Impact
Deprecate
cloneType
for bundles and let the compiler plugin automatically clone bundle fields. Better error message for cases where the plugin is not able to clone types.Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Records now have auto cloneType always enabled.
chisel3.experimental.AutoCloneType
is now deprecated.Reviewer Checklist (only modified by reviewer)
3.4.x
, [small] API extension:3.5.x
, API modification or big change:3.6.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.