-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add zero?
to Number, Time::Span, and Complex
#4026
Conversation
But...why? |
@kirbyfan64 it's a Rubyism. |
The only benefit I see in ruby is to use it in a map / filter. But in crystal it is possible to do [1,0].map &.==(0) # => [false, true] |
Not as pretty as |
I think it's more "human readable" e.g. as opposed to I ran into it while converting some Ruby code into Crystal so I thought I'd add it. It also might save some Rubyists from a few surprises as well. EDIT I agree with @ysbaddaden. IMO it's prettier to write |
@jellymann There is Crystal land, so you need to learn Crystal culture. And, I don't think |
zero?
to Numberzero?
to Number and Time::Span
zero?
to Number and Time::Spanzero?
to Number, Time::Span, and Complex
src/time/span.cr
Outdated
@@ -286,6 +286,10 @@ struct Time::Span | |||
def self.zero | |||
new(0) | |||
end | |||
|
|||
def zero? | |||
self == typeof(self).zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply @ticks == 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "more general approach" needlessly allocates new object on the stack, while actually you only need to compare the @ticks
ivar here. There's a reasoning behind it in relation to Number
since it may hold different values not always comparable with 0
— as Int32
. It does not apply here at all though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a general implementation, then it should be on a more general type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how Time::Span
is a general type. There's Time::Span::Zero
constant more suitable for this anyhow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sija I was agreeing with you.
self == typeof(self).zero
is a general implementation because it works on all types with a self.zero implementation. I was saying that there's no point in using a more general implementation like this versus @ticks == 0
, unless you move #zero?
to Object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so there's no point in doing self == typeof(self).zero
if it's not on a general type. Number is a general type, so it's probably okay to leave it. Time::Span and Complex could probably be more specific, though, since they're concrete types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we create a Zeroable
module that we just include on every type that has a self.zero
implementation? That also allows people to juste drop in Zeroable
on their own classes if they need to.
e.g.
class Vector3
def self.zero
new 0, 0, 0
end
include Zeroable
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work, although it seems a bit verbose to have a module just for that.
src/number.cr
Outdated
# 5.zero? #=> false | ||
# ``` | ||
def zero? : Bool | ||
self == typeof(self).zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply self.to_f == 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For struct Number
is ok the typeof(self).zero
since the literal expanded would be of exact type and that is better. But on Complex and Timespan I would say to go for the plain/@ivar ==
approach. The compiler will end up inlining and simplifying it, but I think is simpler the @ivar ==
and we avoid the extra work in the optimization.
@kirbyfan64 Is Crystal's goal to save keystrokes? You’d think that a language inspired by Ruby would actively embrace Rubyisms. |
We don't want to copy every rubyism. For example we avoid aliases and prefer a single way or method to do something. To add a new feature into the core/stdlib we need to make sure it has a real, added, value. |
One benefit of this would be slightly more type safety because in |
Since using Yet, if an implementation is supposed to receive a |
I'd just like to say, another reason why I think this is a good fit is that Also, |
I also think |
Bump, what's the state of this PR? (the build error is about file formatting) |
I vote for this pull request. Zero has special meaning, and need Formatting should be updated due to changes in crystal formatter itself. |
@jellymann are you available to fix the formatting so this can be merged? |
7564d1b
to
3502fec
Compare
Removed generic implementation of `zero?` to avoid unecessary optimisation.
3502fec
to
abc10ec
Compare
@bcardiff I've fixed the formatting issues, but the Travis build is still failing for some reason. |
Thanks @jellymann! |
No description provided.