Skip to content

RawModule and Module aren't abstract #3534

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seldridge
Copy link
Member

Change RawModule and Module to be "classes" and not "abstract classes". Neither has abstract members.

Change RawModule and Module to be "classes" and not "abstract classes".
Neither has abstract members.

Signed-off-by: Schuyler Eldridge <[email protected]>
@seldridge seldridge added the No Release Notes Exclude from release notes, consider using Internal instead label Sep 18, 2023
@jackkoenig
Copy link
Contributor

If you make this change, then people can just write val module = Module(new RawModule), is this something we want?

@jackkoenig
Copy link
Contributor

They may not be abstract in the strict Scala sense, but they're generally abstract in the "Chisel sense" that you're expected to provide some ports and functionality.

@seldridge
Copy link
Member Author

A user can do Module(new RawModule{}) today which is odd, but allowable. Maybe the extra safeguard against this is helpful? If we're really trying to guard against module creation with empty IO/empty body, then a better, run-time mechanism is likely warranted.

I'm not sure what to do here. This was just surprising as I wasn't expecting to see abstract classes that had no abstract members. This would have been less surprising if this was a trait.

I leave it up to you to decide what makes sense here.

@jackkoenig
Copy link
Contributor

If we're really trying to guard against module creation with empty IO/empty body

I'm not necessarily advocating for active guards, I'm more thinking about code clarity. To me, them being abstract makes it clear you are expected to extend them. If they were merely classes, it implies you can just instantiate them.

I'm not sure what to do here. This was just surprising as I wasn't expecting to see abstract classes that had no abstract members.

That's fair. My intuition is that only Scala experts would be surprised by that while there's some marginal value in the use of abstract telling regular Chisel users that these are intended to be extended rather than instantiated directly.

This would have been less surprising if this was a trait.

Fair, but unfortunately traits can't take arguments in Scala. Currently RawModule and Module do not, but @fabianschuiki just requested that we propagate the source locators through so I think we need to add those arguments 😅.

I leave it up to you to decide what makes sense here.

Given this discussion, the best solution would be a Scala 3 open class. In lieu of that, on the balance I lean slightly toward keeping them abstract but I'm also okay with just making them regular classes. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Release Notes Exclude from release notes, consider using Internal instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants