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

asBools, asBool, and chained apply on asBools #950

Merged
merged 3 commits into from
Dec 4, 2018
Merged

Conversation

jackkoenig
Copy link
Contributor

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

  • toBools -> asBools (toBools deprecated)
  • toBool -> asBool (toBool deprecated)
  • Fix chaining of apply after .asBools

@jackkoenig jackkoenig added API Modification Bugfix Fixes a bug, will be included in release notes Merge with merge commit Please merge with a merge commit, do not squash and merg labels Nov 21, 2018
@jackkoenig jackkoenig requested a review from a team as a code owner November 21, 2018 23:41
@jackkoenig jackkoenig changed the title As bools asBools, asBool, and chained apply on asBools Nov 21, 2018
@@ -78,6 +78,15 @@ abstract class Element extends Data {
*/
private[chisel3] sealed trait ToBoolable extends Element {

/** Casts this $coll to a [[Bool]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is $coll meant to be substituted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seldridge added this when he improved the ScalaDoc, I just copied it from toBool

Copy link
Contributor

@edwardcwang edwardcwang left a comment

Choose a reason for hiding this comment

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

Generally LGTM

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

width match {
case KnownWidth(1) => this(0)
case _ => throwException(s"can't covert UInt<$width> to Bool")
case _ => throwException(s"can't covert ${this.getClass.getSimpleName}$width to Bool")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a future todo: add a function to render a data type as a string. This looks like it's asking for something at a higher level of abstraction and more general that should exist, but doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think a way to render unbound types as well as bound values in a more useful way would be very valuable. The whole UInt@4fd5e7a is pretty worthless most of the time

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a tracking issue for that in #953

@jackkoenig jackkoenig added this to the 3.2.0 milestone Nov 29, 2018
@jackkoenig
Copy link
Contributor Author

retest this please

The expanded version substituted in by the macro was misspelled, renamed
from toBools -> do_toBools as expected by the macro
@jackkoenig jackkoenig merged commit 83f2a65 into master Dec 4, 2018
@jackkoenig jackkoenig deleted the as-bools branch December 5, 2018 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Modification Bugfix Fixes a bug, will be included in release notes Merge with merge commit Please merge with a merge commit, do not squash and merg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants