Warn on <literal>.asUInt|.asSInt(_: Int) #4764
Conversation
The user probably forgot .W. Apply the same technique as used for .U|.S.
jackkoenig
added
the
Deprecation
|
what is the actual change in behavior here? Without a test it's hard to tell |
val x: Int = 3
x.asUInt(8) // this is now a compile error
4.asUInt(10) // this is now a compile errorFull example: class Foo(a: Int) extends Module {
val w, x, y, z = IO(Output(UInt(8.W)))
w := a.asUInt(8)
x := 4.asUInt(8)
y := 5.asUInt(8.W) // this is fine
z := VecInit(1.U, 2.U, 3.U).asUInt // this is fine
}This warns with (but I have warnings as errors so its an error): |
|
what about what's the argument for keeping this fine? |
Typically, the number you put is a width greater than the automatic width of the argument and you'll get an out-of-bounds index error. So basically, it's not usually fine. That doesn't apply to literals for various reasons, arguably if you don't give them a width, they shouldn't really have a width so it's weird to enforce the minimum possible width on a subsequent bit extract. And subsequent bit extracts do sometimes happen, usually in loops or more complex control flow. The main issue though, is that it's really hard to ban a chained apply after anything. @sequencer has suggested getting rid of apply for bit extraction, we could require everyone to explicitly write But rather than trying to tackle something so big, I figured I'd apply the fix we already have for |
|
Looks great to me! To be clear, this is emitted as a Scala warning, not a Chisel warning (as in this), correct? Presumably because we don't have access to the |
Correct, Scala warnings are also configurable, just with an argument to scalac instead of to Chisel. We could make this a Chisel time warning/error but IMO Scala-time is always best when possible and practical. |
The user probably forgot .W. Apply the same technique as used for .U|.S. (cherry picked from commit 811bb46)
✅ Backports have been created
|
Fixes #4733
Unfortunately, this is really hard to write a unit test for, but I did manually check it works (and it's using the same approach as we have been using for
.Uand.S). I'm also not sure that this is the best fix for 4733, but it works.FYI @tymcauley
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
The user probably forgot .W. Apply the same technique as used for .U|.S.
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.