8000 Switch from veneers to private macro interfaces by jackkoenig · Pull Request #4768 · chipsalliance/chisel · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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 23 commits into from
Mar 15, 2025

Conversation

jackkoenig
Copy link
Contributor
@jackkoenig jackkoenig commented Mar 4, 2025

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:

  • In src/main/scala create FooImpl.scala which contains private[chisel3] trait FooImpl and/or private[chisel3] trait ObjectFooImpl (for objects).
    • FooImpl should contain all functional code and package private or protected methods to be called by the public APIs.
    • Any public APIs that don't need macros are also defined here.
  • In src/main/scala-2 and src/main/scala-3, create Foo.scala which contains class Foo extends FooImpl and/or object Foo extends ObjectFooImpl.
    • Foo contains all public APIs that need macros and should contain no implementation code--it should call the private methods defined in FooImpl

The new approach is as follows:

  • In src/main/scala/Foo.scala define class Foo extends FooIntf and/or object Foo extends Foo$Intf.
    • These are the actual public classes/objects.
    • This is where all implementation code must live, including implementing private or protected methods that will be called by the Intf
  • In src/main/scala-2 and src/main/scala-3 define FooIntf.scala which contains private[chisel3] trait FooIntf and/or private[chisel3] trait Foo$Intf.
    • These traits should define any public methods that use macros (or in Scala 3, polyfill macro methods). The implementation of these public methods should be simply to call an _impl version of the same method (defined in the public API in src/main/scala).
    • These traits should be self-typed on the public API that extends them (e.g. { 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 the Intf traits)

The new approach has some advantages over the old approach:

  • The "main" file name for an API is now also where the bulk of the code lives. It was annoying going to Module.scala and that just being the public methods
  • The filename and class/object/trait name where the implementation lives shows up in stack traces, so it's much better to see Module and Module.scala than ObjectModuleImpl and ModuleImpl.scala in a stack trace.
  • The new naming scheme for objects (using trailing $ rather an preceding Object) is more clear IMO as it matches the desugared type name for objects.
  • The self-typing enforces that Intf traits are only mixed in the right place and somewhat self-documents what the Intf traits are for.
  • It avoids duplication of ScalaDoc on public classes/objects.

Draft for now since this needs to be applied to a lot more files.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did 8000 you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@jackkoenig jackkoenig added the Internal Internal change, does not affect users, will be included in release notes label Mar 4, 2025
@jackkoenig jackkoenig mentioned this pull request Mar 4, 2025
14 tasks
@jackkoenig jackkoenig added this to the 7.0 milestone Mar 4, 2025
@jackkoenig jackkoenig force-pushed the jackkoenig/veneer-to-intf branch from e8cddfb to 5ca13d8 Compare March 4, 2025 21:50
@jackkoenig jackkoenig changed the title Switch Module from veneer to private interface Switch from veneers to private macro interfaces Mar 4, 2025
@jackkoenig jackkoenig force-pushed the jackkoenig/veneer-to-intf branch 3 times, most recently from 1b2ea5c to 41b05cb Compare March 11, 2025 00:19
@jackkoenig jackkoenig force-pushed the jackkoenig/veneer-to-intf branch 2 times, most recently from e95b25b to 8fa6263 Compare March 14, 2025 21:02
@jackkoenig jackkoenig force-pushed the jackkoenig/veneer-to-intf branch from a74797e to 0bdd14f Compare March 14, 2025 22:47
@jackkoenig jackkoenig requested a review from seldridge March 14, 2025 22:48
@jackkoenig jackkoenig marked this pull request as ready for review March 14, 2025 22:48
Copy link
Member
@seldridge seldridge left a 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. 🙏

@jackkoenig jackkoenig merged commit 843589c into main Mar 15, 2025
15 checks passed
@jackkoenig jackkoenig deleted the jackkoenig/veneer-to-intf branch March 15, 2025 17:36
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.
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
0