Add BundleLiteralsSpec #37
Conversation
Three simple tests showing syntax and usage Third tests showing peeking currently expects an exception
There was a problem hiding this comment.
cloneType is an old, crappy, legacy, abstraction-breaking, and informally-deprecated API that needs to be killed with fire, then nuked from orbit. It's the only way to be sure.
(rationale: can you explain what cloneType does from the name without going into the guts of Chisel? It just seems to encourage cargo cult programming, and chiselTypeOf expresses intent much better)
Also potentially questionable style issues for the ignored test.
| @@ -36,35 +25,44 @@ class BundleLiteralSpec extends FlatSpec with ChiselScalatestTester { | |||
| io.aOut := io.in.a | |||
| io.bOut := io.in.b | |||
| }) { c => | |||
| c.io.in.poke(c.io.in.Lit(0.U, 1.U)) | |||
| c.io.in.poke(c.io.in.cloneType.Lit(_.a -> 0.U, _.b -> 1.U)) | |||
There was a problem hiding this comment.
cloneType is a crappy, confusing, and informally-deprecated API. Please stop using it, or encouraging people to use it, or throwing it in new code. We can't actually mark it deprecated, because some legacy code makes heavy use of it, but new code should not use it.
Use chiselTypeOf instead.
There was a problem hiding this comment.
Agreed, I meant to ask you about this, but forgot about it. Fixed
| } | ||
| } | ||
| } | ||
|
|
||
| ignore should "peek Bundle literals" in { | ||
| // TODO to be written | ||
| // peek on bundle not supported yet, this test will fail and should |
There was a problem hiding this comment.
Why'd you get rid of the ignore? I think the ignore is cleaner and expresses intent better than an exception intercept. The test name is also very confusing.
There was a problem hiding this comment.
I think getting rid of the ignore is a better way of making sure we update the test when peeking is of Bundled is enabled. I agree the test name is bad and is changed.
There was a problem hiding this comment.
I'd prefer expressing intent over some confusing system that results in automated reminders. Because when you're building a new feature, you should already be thinking of writing tests for it, and the ignored should act as a flag that gets cleaned up (though perhaps eventually). And the tests are intended to essentially codify behavior, and a LiteralTypeException is not the end goal here.
I guess ideally there would be an ignore flag that checks for a test failure and suppresses it, but errors on a non-failure. Don't know if that exists in ScalaTest.
There was a problem hiding this comment.
Test is now an ignore
| test(new PassthroughModule(new DoubleElements)) { c => | ||
| c.in.poke(c.in.cloneType.Lit(_.a -> 0.U, _.b -> 1.U)) | ||
| val output = c.out.peek() | ||
| output.a === 0.U should be(true.B) |
There was a problem hiding this comment.
Uh, I really don't think that will work, since we don't do const prop in the frontend - it will just end up as a wire, and that's assuming you don't get some error for trying to invoke HCL operations outside a Builder context.
That being said, I'm not sure how we check for literal equality.
There was a problem hiding this comment.
@ducky64 Can you think of a better example of a use case for peeking a Bundle?
There was a problem hiding this comment.
Test is now an ignore
Use `chiselTypeOf` instead of `cloneType`. Fix name of putative test of peeking Bundles
peeking bundles is not supported yet
Three simple tests showing syntax and usage
Third tests showing peeking currently expects an exception