-
Notifications
You must be signed in to change notification settings - Fork 631
Switch from veneers to private macro interfaces #4768
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e8cddfb
to
5ca13d8
Compare
1b2ea5c
to
41b05cb
Compare
e95b25b
to
8fa6263
Compare
a74797e
to
0bdd14f
Compare
seldridge
approved these changes
Mar 15, 2025
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 glanced though all the files. I'm good with the pattern.
I'm really hoping that we can get this Scala3 migration done quickly so that all this can be cleaned up. 🙏
tymcauley
added a commit
to tymcauley/fixedpoint
that referenced
this pull request
Mar 24, 2025
`UnknownWidth` became a `case object` in this Chisel PR: chipsalliance/chisel#4242 The `connectFromBits` method was removed in this Chisel PR: chipsalliance/chisel#4168 The `connectFromBits` API was replaced with the `_fromUInt` API in this Chisel PR: chipsalliance/chisel#4782 The `NumObject` trait was replaced by the `Num` object in this PR: chipsalliance/chisel#4768 The sbt update is necessary to maintain compatibility with the latest Scala compiler version.
tymcauley
added a commit
to tymcauley/fixedpoint
that referenced
this pull request
Mar 24, 2025
`UnknownWidth` became a `case object` in this Chisel PR: chipsalliance/chisel#4242 The `connectFromBits` method was removed in this Chisel PR: chipsalliance/chisel#4168 The `connectFromBits` API was replaced with the `_fromUInt` API in this Chisel PR: chipsalliance/chisel#4782 The `NumObject` trait was replaced by the `Num` object in th 8000 is PR: chipsalliance/chisel#4768 The sbt update is necessary to maintain compatibility with the latest Scala compiler version.
tymcauley
added a commit
to tymcauley/fixedpoint
that referenced
this pull request
Apr 2, 2025
`UnknownWidth` became a `case object` in this Chisel PR: chipsalliance/chisel#4242 The `connectFromBits` method was removed in this Chisel PR: chipsalliance/chisel#4168 The `connectFromBits` API was replaced with the `_fromUInt` API in this Chisel PR: chipsalliance/chisel#4782 The `NumObject` trait was replaced by the `Num` object in this PR: chipsalliance/chisel#4768 Wrapping unary negation (`unary_-%`) was deprecated in this PR: chipsalliance/chisel#4829 The sbt update is necessary to maintain compatibility with the latest Scala compiler version.
tymcauley
added a commit
to tymcauley/chisel
that referenced
this pull request
Apr 2, 2025
These were accidentally removed in chipsalliance#4768.
14 tasks
jackkoenig
pushed a commit
that referenced
this pull request
Apr 2, 2025
These were accidentally removed in #4768.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an update to the "Scala 2/3 veneer pattern" for how to deal with public methods that use macros, first described in https://github.com/chipsalliance/chisel/pull/4682/files#r1979854957.
The old pattern was as follows:
src/main/scala
createFooImpl.scala
which containsprivate[chisel3] trait FooImpl
and/orprivate[chisel3] trait ObjectFooImpl
(forobjects
).FooImpl
should contain all functional code and package private or protected methods to be called by the public APIs.src/main/scala-2
andsrc/main/scala-3
, createFoo.scala
which containsclass Foo extends FooImpl
and/orobject Foo extends ObjectFooImpl
.Foo
contains all public APIs that need macros and should contain no implementation code--it should call the private methods defined inFooImpl
The new approach is as follows:
src/main/scala/Foo.scala
defineclass Foo extends FooIntf
and/orobject Foo extends Foo$Intf
.Intf
src/main/scala-2
andsrc/main/scala-3
defineFooIntf.scala
which containsprivate[chisel3] trait FooIntf
and/orprivate[chisel3] trait Foo$Intf
._impl
version of the same method (defined in the public API insrc/main/scala
).{ self: Foo =>
or{ self: Foo.type =>
for objects). (this is a circular dependency but it enables calling implementation methods defined in the public API)(Note: originally "new approach" included
FooVirtualMethods
, but this is not actually necessary due to the self typing on theIntf
traits)The new approach has some advantages over the old approach:
Module.scala
and that just being the public methodsModule
andModule.scala
thanObjectModuleImpl
andModuleImpl.scala
in a stack trace.$
rather an precedingObject
) is more clear IMO as it matches the desugared type name for objects.Draft for now since this needs to be applied to a lot more files.
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)
and clean up the commit message.Create a merge commit
.