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.

Sign up for GitHub

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

Jump to bottom

Bool: Add to_unsafe #5465

Merged
merged 1 commit into from Jan 2, 2018
Merged

Bool: Add to_unsafe #5465

merged 1 commit into from Jan 2, 2018

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Dec 27, 2017 •
edited

Introduces Bool.to_unsafe, which returns 1 for true and 0 for false.

This comes in handy when calling C bindings that take a bool-ish integer, and is clearer than Bool.hash (which also changed in 0.24.1):

def whatever(foo, bar, *, baz = true)
  # frobulate(char* foo, type_t bar, int baz)
  LibWhatever.frobulate(foo, bar, baz.to_unsafe)
end

@RX14
Copy link
Contributor

RX14 commented Dec 27, 2017

I'd prefer the naming to_unsafe, despite it not being entirely correct. We've discussed that booleans don't have an ordering before so naturally I think that extends to we shouldn't encourage people to see booleans as numbers.

@woodruffw woodruffw changed the title Bool: Add to_i Bool: Add to_unsafe Dec 27, 2017
@Wulfklaue
Copy link

I think that @woodruffw his to_i is more clear to understand. The to_unsafe is a keyword is better left to actions that really are unsafe.

@woodruffw
Copy link
Contributor Author

I agree with @RX14 -- to_i might be more correct, but treating a boolean as an integer anywhere in (pure) Crystal is probably a code smell. to_unsafe expresses that more explicitly, even if it's a perfectly safe operation from the compiler/runtime's perspective.

@RX14
Copy link
Contributor

RX14 commented Dec 27, 2017

Has the added benefit of allowing you to pass boolean variables into functions without conversion - the to_unsafe is automatic.

@larubujo
Copy link
Contributor

probably better let compiler accept boolean in c binding

RX14
RX14 requested changes Dec 28, 2017
View reviewed changes
src/bool.cr Outdated
@@ -46,6 +46,11 @@ struct Bool
hasher.bool(self)
end

# Returns `1` for `true` and `0` for `false`.
def to_i
Copy link
Contributor

Choose a reason for hiding this comment

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

You renamed the PR but didn't push new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's weird, I thought I did. Pushed it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, that was stupid of me. I amended the commit but didn't actually re-add my changes. Sorry about that.

@woodruffw
Bool: Add to_unsafe
2e03122 
`Bool.to_unsafe` returns `1` for `true` and `0` for false.
@asterite
Copy link
Member

asterite commented Jan 2, 2018

There's nothing unsafe from going from Bool to Int32. I agree with @larubujo here, bools should be allowed in C bindings (I tried to implement that once but failed, but I'm sure it can be done).

@RX14
Copy link
Contributor

RX14 commented Jan 2, 2018

@asterite I agree there's noting unsafe about going Bool to Int32 but I think it's the best thing to do here.

Also implementing Bool in C bindings is weird to me, as bool isn't a datatype in C, it's only implemented as an integer.

asterite
asterite approved these changes Jan 2, 2018
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Let's go with this for now

@RX14 RX14 added this to the Next milestone Jan 2, 2018
@RX14 RX14 merged commit 56b8edd into crystal-lang:master Jan 2, 2018
@woodruffw woodruffw deleted the bool-to-i branch January 2, 2018 16:43
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
@woodruffw @lukeasrodgers
Bool: Add to_unsafe (crystal-lang#5465)
a8c1967 
`Bool.to_unsafe` returns `1` for `true` and `0` for false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asterite asterite asterite approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@woodruffw @RX14 @Wulfklaue @larubujo @asterite