add curried MuxLookup.apply, deprecate old apply#3095
Conversation
mwachs5
left a comment
mwachs5
left a comment
There was a problem hiding this comment.
Are you intentionally relaxing the constraint that the enumerated should all be of the same type?
| assert(MuxLookup.fromEnum(TestEnum.c, 3.U)(incompleteMapping) === 2.U) | ||
| assert(MuxLookup(TestEnum.a, 3.U)(incompleteMapping) === 0.U) | ||
| assert(MuxLookup(TestEnum.b, 3.U)(incompleteMapping) === 3.U) | ||
| assert(MuxLookup(TestEnum.c, 3.U)(incompleteMapping) === 2.U) |
There was a problem hiding this comment.
Like a case where I define anotherEnum I should not be able to put AnotherEnum.c in for the default or any of the cases, I think.
There was a problem hiding this comment.
Agreed--we really need a testing flow for negative compile tests...
I have a prototype of the build flow to make this work, but we need some pithy way to express what we expect. Either a standalone FileCheck, a simpler custom similar thing, or perhaps Linux expect
There was a problem hiding this comment.
we could use a macro like the one shapeless uses https://github.com/milessabin/shapeless/blob/main/core/shared/src/main/scala/shapeless/test/typechecking.scala
There was a problem hiding this comment.
ScalaTest has something similar, but macros don't work super well because they aren't full separate invocations of the compiler and thus can't take into account differing compile options
|
Since this deletes an API add in #3071 and that was backported to 3.6.x and 3.5.x, we need to backport this. As is, this won't be binary compatible because of the change to the deprecated apply method, but that can be fixed in the 3.5.x backport (we can just remove the use of the macro and it won't have a good source locator--it's fine). |
jackkoenig
added
the
Please Merge
|
I pushed a commit that makes sure the |
* overload MuxLookup.apply for EnumType * remove MuxLookup.fromEnum * add source locators to MuxLookup.apply using macros (cherry picked from commit dcecabb)
* overload MuxLookup.apply for EnumType * remove MuxLookup.fromEnum * add source locators to MuxLookup.apply using macros (cherry picked from commit dcecabb)
* overload MuxLookup.apply for EnumType * remove MuxLookup.fromEnum * add source locators to MuxLookup.apply using macros (cherry picked from commit dcecabb) Co-authored-by: Albert Chen <40366337+albertchen-sifive@users.noreply.github.com>
) * add curried MuxLookup.apply, deprecate old apply (#3095) * overload MuxLookup.apply for EnumType * remove MuxLookup.fromEnum * add source locators to MuxLookup.apply using macros (cherry picked from commit dcecabb) * remove deprecated annotation, remove macro --------- Co-authored-by: Albert Chen <40366337+albertchen-sifive@users.noreply.github.com> Co-authored-by: Albert Chen <albert.chen@sifive.com>
3 participants
fixes #2780
Contributor Checklist
docs/src?Type of Improvement
new feature/API
API Impact
deprecates the current
MuxLookup.applyand adds two overloads which behave the same as the oldMuxLookup.applybut take two parameter lists instead.MuxLookup.fromEnumis replaced with the newMuxLookup.apply.Backend Code Generation Impact
none, but should generate better source locators for
MuxLookup.applyDesired Merge Strategy
Squash
Release Notes
Add a new version of
MuxLookup.applythat takes two parameter lists instead of one. This helps the scala compiler report better type errors.Reviewer Checklist (only modified by reviewer)
3.5.xor3.6.xdepending on impact, API modification or big change:5.0.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.