-
Notifications
You must be signed in to change notification settings - Fork 298
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
[FIRRTL] Hoist common type-property methods up to FIRRTLType. #5125
Conversation
if (!baseType) | ||
return false; | ||
|
||
return !baseType.isPassive() || baseType.containsAnalog(); |
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.
Limiting "isInOut" to FIRRTLBaseType (~= "hardware type") works presently but conceivably other non-hardware-only types might want different answers.
For example, a non-hw aggregate type mixing hardware and not, this (and "is passive") is less clear, to be tackled when adding them (but heads-up). Conservatively saying "no" for non-hw types (as we currently do).
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
Split out queries that are reasonable for all FIRRTLType's, leave the rest in FIRRTLBaseType. Notably: * RefType is not passive, nor ground. * isPassive() and getPassiveType() only on FIRRTLBaseType. * isConst is supported on all FIRRTLType's. Note that RTP is not stored for all types, but make these properties and basic predicates using them, part of all FIRRTLType's.
Per reviewer feedback, thanks!
Per reviewer feedback, thanks!
b9bcea5
to
d732b1f
Compare
Thanks, folks! |
Split out queries that are reasonable for all FIRRTLType's, leave the rest in FIRRTLBaseType.
Notably:
Note that RTP is not stored for all types, but make these properties and basic
predicates using them, part of all FIRRTLType's.