Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Enum API #385

Merged
merged 4 commits into from
Nov 24, 2016
Merged

Simplify Enum API #385

merged 4 commits into from
Nov 24, 2016

Conversation

ducky64
Copy link
Contributor

@ducky64 ducky64 commented Nov 23, 2016

Get rid of some cruft exposed in #373

This also allows Bits.fromtInt(...) to be removed. Yay!

All old APIs (with some new restrictions, rocket still works fine) are preserved without deprecation in Chisel._, aside from the non-compile-time-checkable Map[] enum constructor which probably should have been deprecated during chisel2. The Map[] enums have been removed from chisel3._ without deprecation.

The new restriction is that nodeType (legacy API) may only be of UInt type with unspecified width. Note that Bits() creates a UInt, and if you can't control the enum values, it makes little sense to specify a bitwidth.

Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

Why are the Symbol-based enums deprecated in the compatibility layer? (I'm not aware of anyone who uses them; I'm just trying to understand why it is OK to remove features from the compatibility layer.)

Otherwise looks great.

@ducky64
Copy link
Contributor Author

ducky64 commented Nov 23, 2016

Scala can't check a map access during compile time (unlike the List-based version, which turn into vals that the compiler understands). This just nags people who are still using that to do something safer, I have no plans to actually remove those elements from the compatibility layer.

If you're super against it I'll remove the deprecation tags.

@aswaterman
Copy link
Member

I was just trying to understand the rationale. I guess I agree they should be deprecated, even in the compatibility layer.

@ducky64
Copy link
Contributor Author

ducky64 commented Nov 23, 2016

I'll document the rationale and that those are not to be removed.

@ducky64 ducky64 merged commit edb19a0 into master Nov 24, 2016
@ducky64 ducky64 deleted the depenums branch November 24, 2016 00:01
@seldridge
Copy link
Member

I'm only just seeing this from a rocket-chip bump on my end.

I'm against the deprecation tags here. Unless something has been fixed upstream with Scala, the Tuple-based approach for Enums hits Scala's limit of 22 on Tuple sizes. The only way that I know of to get around this is to use the List-based version (ignoring the fact that I idiotically have a legacy state machine with > 22 states...).

Thanks for keeping this support in, nonetheless!

mwachs5 pushed a commit that referenced this pull request Jan 15, 2023
Bumps [firrtl](https://github.com/freechipsproject/firrtl) from `01e567d` to `e97c7b1`.
- [Release notes](https://github.com/freechipsproject/firrtl/releases)
- [Commits](chipsalliance/firrtl@01e567d...e97c7b1)

---
updated-dependencies:
- dependency-name: firrtl
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants