Add `zero?` to Number, Time::Span, and Complex #4026

danini-the-panini · 2017-02-12T14:26:05Z

No description provided.

refi64 · 2017-02-12T14:31:17Z

But...why? x.zero? is even a character longer than x == 0, so I'm not seeing any real benefit...

0x1eef · 2017-02-12T18:24:46Z

bcardiff · 2017-02-12T18:33:26Z

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]

ysbaddaden · 2017-02-12T20:32:31Z

Not as pretty as ary.reject(&.zero?) but more flexible since it can take any value.

danini-the-panini · 2017-02-12T21:00:29Z

I think it's more "human readable"

e.g.
do_the_thing until value.zero?

as opposed to
do_the_thing until value == 0

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 ary.reject(&.zero?) than ary.reject &.==(0). It also "reads" more like English.

makenowjust · 2017-02-13T08:21:31Z

@jellymann There is Crystal land, so you need to learn Crystal culture.

And, I don't think zero? is bad idea when I call Number.zero and another T.zero to mind (T is a class having zero class method). The predicate to check equality with T.zero should be named zero?, and it can't simply replace x == 0 (Of course, we can replace it as x == typeof(x).zero). In fact Time::Span has zero class method and its instance can't compare with 0. You should approach it with Time::Span#zero?.

Sija · 2017-02-13T12:33:32Z

src/time/span.cr

@@ -286,6 +286,10 @@ struct Time::Span
  def self.zero
    new(0)
  end
+
+  def zero?
+    self == typeof(self).zero


why not simply @ticks == 0?

Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"

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.

If it's a general implementation, then it should be on a more general type.

I don't see how Time::Span is a general type. There's Time::Span::Zero constant more suitable for this anyhow.

@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.

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.

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

That could work, although it seems a bit verbose to have a module just for that.

Sija · 2017-02-13T12:33:59Z

src/number.cr

+  # 5.zero? #=> false
+  # ```
+  def zero? : Bool
+    self == typeof(self).zero


why not simply self.to_f == 0?

Read @makenowjust's comment. This is a more general approach, comparing directly to the type's definition of "zero"

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.

danini-the-panini · 2017-02-17T14:00:21Z

@kirbyfan64 Is Crystal's goal to save keystrokes?

You’d think that a language inspired by Ruby would actively embrace Rubyisms.

ysbaddaden · 2017-02-17T14:23:07Z

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.

JacobUb · 2017-02-17T22:05:08Z

One benefit of this would be slightly more type safety because in x == 0, "x" could, by mistake, be of some type with no meaningful comparison to a number and x.zero? would throw a compiler error instead.

bcardiff · 2017-03-18T02:44:22Z

Since using #zero? is funner and I see no cons on adding this I am open to include it.

Yet, if an implementation is supposed to receive a number (scalar or complex), checking with == 0 is enough. Even if typeof(0) != typeof(number). So the only value of adding this is to have some unified way to distinguish numbers and timespans zeros. If anyone can highlight a benefit for this it might help on others to 👍 or 👎 .

danini-the-panini · 2017-03-25T14:09:19Z

I'd just like to say, another reason why I think this is a good fit is that Int already has even? and odd?, and I think zero? fits well into that group of functions.

Also, Complex#zero? is easier than x.real == 0 && x.imag == 0, and more concise than x == Complex.zero. Similarly for time spans, consider x.ticks == 0 or x == Time::Span.zero vs x.zero?. I think it's pretty good reason for zero? to exist, and in going with POLA, Number ought to have it as well.

ozra · 2017-03-28T23:42:26Z

I also think zero? makes sense (as an "almost anti ruby" dude at that). Zero is not "just another number". And as already mentioned, in generic code one call covers a bunch of ugly situations. I've used the corresponding thing in C++ for generic containers/readers etc. [ed: for some rather specialized datatypes where 0 still "is a thing"] and I imagine I'll stumble in those directions in Crystal too. +1.

bew · 2017-06-09T21:11:15Z

Bump, what's the state of this PR? (the build error is about file formatting)

akzhan · 2017-06-14T10:48:25Z

I vote for this pull request.

Zero has special meaning, and need zero? just like infinity? and nan?.

Formatting should be updated due to changes in crystal formatter itself.

bcardiff · 2017-06-14T13:29:21Z

@jellymann are you available to fix the formatting so this can be merged?

Removed generic implementation of `zero?` to avoid unecessary optimisation.

danini-the-panini · 2017-06-22T21:26:59Z

@bcardiff I've fixed the formatting issues, but the Travis build is still failing for some reason.

Sija · 2017-06-22T21:46:05Z

@bcardiff C'mon, lets merge #4528...

bcardiff · 2017-06-23T12:20:29Z

Thanks @jellymann!

danini-the-panini changed the title ~~Add zero? to Number~~ Add zero? to Number and Time::Span Feb 13, 2017

danini-the-panini changed the title ~~Add zero? to Number and Time::Span~~ Add zero? to Number, Time::Span, and Complex Feb 13, 2017

Sija reviewed Feb 13, 2017

View reviewed changes

mverzilli added kind:feature topic:stdlib labels Jun 15, 2017

Daniel Smith added 3 commits June 22, 2017 20:19

Add zero? to Number

cf84047

Add zero? method to Time::Span

0b14f1f

Add zero? method to Complex

93bd8e1

danini-the-panini force-pushed the patch-1 branch from 7564d1b to 3502fec Compare June 22, 2017 18:21

Change zero? implementation

abc10ec

Removed generic implementation of `zero?` to avoid unecessary optimisation.

danini-the-panini force-pushed the patch-1 branch from 3502fec to abc10ec Compare June 22, 2017 19:43

Fix formatting regarding newlines

4b5d91a

bcardiff merged commit a431a57 into crystal-lang:master Jun 23, 2017

bcardiff added this to the Next milestone Jun 23, 2017

danini-the-panini deleted the patch-1 branch June 23, 2017 12:56

Sija referenced this pull request Jun 27, 2017

Crystal 0.23.0 Changelog

3c3d3e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `zero?` to Number, Time::Span, and Complex #4026

Add `zero?` to Number, Time::Span, and Complex #4026

danini-the-panini commented Feb 12, 2017

refi64 commented Feb 12, 2017

0x1eef commented Feb 12, 2017

bcardiff commented Feb 12, 2017

ysbaddaden commented Feb 12, 2017

danini-the-panini commented Feb 12, 2017 •

edited

makenowjust commented Feb 13, 2017

Sija Feb 13, 2017 •

edited

danini-the-panini Feb 13, 2017

Sija Feb 13, 2017 •

edited

RX14 Feb 13, 2017

Sija Feb 13, 2017 •

edited

RX14 Feb 13, 2017 •

edited

danini-the-panini Feb 13, 2017

danini-the-panini Feb 13, 2017

RX14 Feb 13, 2017

Sija Feb 13, 2017 •

edited

danini-the-panini Feb 13, 2017

bcardiff Mar 18, 2017

danini-the-panini commented Feb 17, 2017

ysbaddaden commented Feb 17, 2017

JacobUb commented Feb 17, 2017

bcardiff commented Mar 18, 2017

danini-the-panini commented Mar 25, 2017 •

edited

ozra commented Mar 28, 2017 •

edited

bew commented Jun 9, 2017

akzhan commented Jun 14, 2017 •

edited

bcardiff commented Jun 14, 2017

danini-the-panini commented Jun 22, 2017

Sija commented Jun 22, 2017

bcardiff commented Jun 23, 2017

Add zero? to Number, Time::Span, and Complex #4026

Add zero? to Number, Time::Span, and Complex #4026

Conversation

danini-the-panini commented Feb 12, 2017

refi64 commented Feb 12, 2017

0x1eef commented Feb 12, 2017

bcardiff commented Feb 12, 2017

ysbaddaden commented Feb 12, 2017

danini-the-panini commented Feb 12, 2017 • edited

makenowjust commented Feb 13, 2017

Sija Feb 13, 2017 • edited

Choose a reason for hiding this comment

danini-the-panini Feb 13, 2017

Choose a reason for hiding this comment

Sija Feb 13, 2017 • edited

Choose a reason for hiding this comment

RX14 Feb 13, 2017

Choose a reason for hiding this comment

Sija Feb 13, 2017 • edited

Choose a reason for hiding this comment

RX14 Feb 13, 2017 • edited

Choose a reason for hiding this comment

danini-the-panini Feb 13, 2017

Choose a reason for hiding this comment

danini-the-panini Feb 13, 2017

Choose a reason for hiding this comment

RX14 Feb 13, 2017

Choose a reason for hiding this comment

Sija Feb 13, 2017 • edited

Choose a reason for hiding this comment

danini-the-panini Feb 13, 2017

Choose a reason for hiding this comment

bcardiff Mar 18, 2017

Choose a reason for hiding this comment

danini-the-panini commented Feb 17, 2017

ysbaddaden commented Feb 17, 2017

JacobUb commented Feb 17, 2017

bcardiff commented Mar 18, 2017

danini-the-panini commented Mar 25, 2017 • edited

ozra commented Mar 28, 2017 • edited

bew commented Jun 9, 2017

akzhan commented Jun 14, 2017 • edited

bcardiff commented Jun 14, 2017

danini-the-panini commented Jun 22, 2017

Sija commented Jun 22, 2017

bcardiff commented Jun 23, 2017

Add `zero?` to Number, Time::Span, and Complex #4026

Add `zero?` to Number, Time::Span, and Complex #4026

danini-the-panini commented Feb 12, 2017 •

edited

Sija Feb 13, 2017 •

edited

Sija Feb 13, 2017 •

edited

Sija Feb 13, 2017 •

edited

RX14 Feb 13, 2017 •

edited

Sija Feb 13, 2017 •

edited

danini-the-panini commented Mar 25, 2017 •

edited

ozra commented Mar 28, 2017 •

edited

akzhan commented Jun 14, 2017 •

edited