Make BaseModule.name lazy #912
Conversation
This changes BaseModule.name to be lazy (instead of eager) to enable a desiredName to be a function of a sub-instance. This includes a test case showing the new behavior. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
There was a problem hiding this comment.
This is definitely an improvement over the status quo so I approve.
Food for thought: It's still possible to get a NullPointerException like in #911 if you ask for name in the Module before it's available (ie. evaluating the lazy val early). Perhaps when evaluating name we should check if desiredName is null and throw a more precise error message?
Error: desiredName of MyModuleClass@1f3d6e4 is null. Perhaps you evaluated "name" before all values accessed by desiredName were available?
|
Hmm... I ran this locally and it passed. Poking around... Good point, @jackkoenig. Let me get a test and include that. |
|
We really need to get the chisel3 Travis/CircleCI testing running |
This wraps the evaluation of BaseModule.name in try/catch to look for a NullPointerException that may result from trying to evaluate desiredName before it's ready. This catches a test case of using a desiredName that depends on a later defined eager subinstance. h/t @jackkoenig Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>
|
@jackkoenig: Pushed a commit that updates this with your message. This catches the case shown in the test. I've requested you re-review this and make sure that this catches what you were thinking. The output of a new, excepting testing case is now: |
|
Ugh... Chisel master is broken with FIRRTL master for Edit: Filed chipsalliance/firrtl#922 |
|
retest this please |
seldridge
force-pushed
the
c911
branch
2 times, most recently
from
f64388e
to
2416d1d
Compare
Fixes #911
This makes
BaseModule.namelazy (previously eager) so that a Module'sdesiredNamecan be a function of some property associated with a sub-instance.Related issue:
Type of change: other enhancement
Impact: no functional change
Development Phase: implementation
Release Notes