chisel
chisel copied to clipboard
chipsalliance
RawModule and Module aren't abstract
Change RawModule and Module to be "classes" and not "abstract classes". Neither has abstract members.
If you make this change, then people can just write val module = Module(new RawModule), is this something we want?
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.
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.
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?