Skip to content

add MuxLookup.fromEnum#3071

Merged
albertchen-sifive merged 2 commits intomainfrom
mux-lookup-enum
Mar 9, 2023
Merged

add MuxLookup.fromEnum#3071
albertchen-sifive merged 2 commits intomainfrom
mux-lookup-enum

Conversation

@albertchen-sifive
Copy link
Contributor

@albertchen-sifive albertchen-sifive commented Mar 6, 2023

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

API Impact

adds chisel3.util.MuxLookup.fromEnum

Backend Code Generation Impact

none

Desired Merge Strategy

Squash

Release Notes

add chisel3.util.MuxLookup.fromEnum

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

ekiwi
ekiwi reviewed Mar 6, 2023
View reviewed changes
src/main/scala/chisel3/util/Mux.scala Outdated
* @param mapping a sequence to search of keys and values
* @return the value found or the default if not
*/
def fromEnum[S <: EnumType, T <: Data](key: S, default: T, mapping: Seq[(S, T)]): T = {
Copy link
Contributor

@ekiwi ekiwi Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if you try to overload the apply function? Does the Scala compiler complain?

Copy link
Contributor Author

@albertchen-sifive albertchen-sifive Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a compile error method apply is defined twice. https://scastie.scala-lang.org/eO18f45oQgKIi2GiBhMNYg

Copy link
Contributor

@ekiwi ekiwi Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Sad :(

Copy link
Contributor

@jackkoenig jackkoenig Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're successfully overloading apply now 🙂 see #3095

@mmaloney-sf
Copy link
Contributor

mmaloney-sf commented Mar 6, 2023

Thanks @albertchen-sifive!

Would you mind adding a short code example to how to use this new method in the docstring?

@albertchen-sifive
Copy link
Contributor Author

albertchen-sifive commented Mar 6, 2023

I realized I got the order of operations wrong. I want to also make a PR to fix this issue #2780 (improving the signature of MuxLookup.apply), but this new method is following the old signature. I think I'll change the MuxLookup.fromEnum method to follow the suggested signature in the issue and then make a separate PR that adds a new version of MuxLookup.apply with the deprecation. Is that OK?

@mwachs5
Copy link
Contributor

mwachs5 commented Mar 7, 2023 •
edited
Loading

I think I'll change the MuxLookup.fromEnum method to follow the suggested signature in the issue and then make a separate PR that adds a new version of MuxLookup.apply with the deprecation. Is that OK?

Either order is fine with me! It makes sense to me to only add this new method with the way we want to have the signature in the end.

@albertchen-sifive albertchen-sifive merged commit d574078 into main Mar 9, 2023
@albertchen-sifive albertchen-sifive deleted the mux-lookup-enum branch March 9, 2023 02:24
@mwachs5 mwachs5 added this to the 3.5.x milestone Mar 9, 2023
@mergify mergify bot added the Backported This PR has been backported label Mar 9, 2023
This was referenced Mar 9, 2023
Merged
Merged
mergify bot added a commit that referenced this pull request Mar 10, 2023
@mergify @albertchen-sifive @mwachs5
add MuxLookup.fromEnum (backport #3071) (#3077)
36e922a 
* add MuxLookup.fromEnum

(cherry picked from commit 6dc2f58)

* add example, better type signature

(cherry picked from commit c1b8e4c)

---------

Co-authored-by: Albert Chen <albert.chen@sifive.com>
Co-authored-by: Megan Wachs <megan@sifive.com>
mergify bot added a commit that referenced this pull request Mar 14, 2023
@mergify @albertchen-sifive
add MuxLookup.fromEnum (backport #3071) (#3076)
fee9d28 
* add MuxLookup.fromEnum

(cherry picked from commit 6dc2f58)

* add example, better type signature

(cherry picked from commit c1b8e4c)

---------

Co-authored-by: Albert Chen <albert.chen@sifive.com>
@jackkoenig jackkoenig mentioned this pull request Mar 17, 2023
Merged
16 tasks
@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Apr 6, 2023
@sequencer sequencer mentioned this pull request Jun 20, 2023
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekiwi ekiwi ekiwi left review comments

@mwachs5 mwachs5 mwachs5 approved these changes

@jackkoenig jackkoenig Awaiting requested review from jackkoenig

Assignees

No one assigned

Labels

Backported This PR has been backported Feature New feature, will be included in release notes

Projects

None yet

Milestone

3.5.x

Development

Successfully merging this pull request may close these issues.

5 participants

@albertchen-sifive @mmaloney-sf @mwachs5 @ekiwi @jackkoenig