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

Cleanup implicit conversions #868

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Cleanup implicit conversions #868

merged 2 commits into from
Jul 31, 2018

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jul 31, 2018

Remove public vals from implicit classes that can surprisingly be
invoked yet do nothing. Many implicit classes were Value Classes which
require the argument to be a public val but the avoidance of runtime
objects compared to the rest of Chisel runtime is minimal at best.

Related issue: NA

Type of change: bug fix

Impact: API modification

This PR is not source-code compatible since someone could be accidentally invoking one of these "methods". We could instead do this via a deprecation schedule.

Development Phase: implementation

Release Notes

Removed implicitly added identity methods .data and .target from Data and DecoupledIO, and removed .x from String, Int, Long, BigInt, and Boolean

Remove public vals from implicit classes that can be confusingly be
invoked yet do nothing. Many implicit classes were Value Classes which
require the argument to be a public val but the avoidance of runtime
objects compared to the rest of Chisel runtime is minimal at best.
Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

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

lgtm - deferring to @ducky64 for approval

Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

lgtm

Not sure why those were vals, especially since none of the common tutorials on implicits have them as vals...

@jackkoenig
Copy link
Contributor Author

So they do have to be vals if it's a Value Class (the ones that extended AnyVal), but the only purpose of Value Classes is to provide an implicit class without runtime boxing. I thought about changing those vals to be self or _self or something but since such runtime boxing is a drop in the bucket of the overfall performance of Chisel, I thought it cleaner just to convert them to standard implicit classes and removes the vals.

@jackkoenig jackkoenig merged commit 4de6848 into master Jul 31, 2018
@jackkoenig jackkoenig deleted the cleanup-implicits branch July 31, 2018 22:43
@ucbjrl ucbjrl added this to the 3.1.3 milestone Sep 7, 2018
ucbjrl added a commit that referenced this pull request Sep 11, 2018
(cherry picked from commit 4de6848)
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