Fix Float#<=> when other responds to infinite?

[stevegeek]

Fixes cmp to support comparing infinity to rhs which responds to infinite?, and updates the spec to current upstream comparison spec.

A quick point for discussion:

here I use ->as_integer() on the Value returned from infinite? (which should return 1, nil, or -1) after checking for nil... however it is possible that an implementation of that method returns a non-integer value. If this is the case an assertion error will occur here.

If we look at Ruby, it handles this by calling rb_cmpint which effectively converts the value into an integer of the set of valid values [-1, 0, 1]. It does this for non-integer values by calling < 0 and > 0 on the object - https://github.com/ruby/ruby/blob/5ccb625fbbd1e774636a9fdbe0bf1c3d38e085d5/bignum.c#L2969

I could implement that same behaviour here in-line, eg I see this is done in-line here for example:

natalie/src/time_object.cpp

Line 97 in 99df60f

if (result->send(env, ">"_s, { Value::integer(0) })->is_true()) {

But I guess a function such as rb_cmpint would be better, to be used generally where return values for use with cmp need converting (eg in Time as linked above).

However I'm not sure where that function would live in natalie, any thoughts welcome!

[stevegeek]

For example, these should pass:

  it "returns 1 when self is Infinity and other infinite? returns value that responds is neither > or < than 0" do
    obj = Object.new
    def obj.infinite?
      val = Object.new
      def val.<(other)
        false
      end
      def val.>(other)
        false
      end
      val
    end
    (infinity_value <=> obj).should == 1
  end

  it "returns -1 when self is -Infinity and other infinite?=1 via value that responds to > and is greater than 0" do
    obj = Object.new
    def obj.infinite?
      val = Object.new
      def val.>(other)
        true
      end
      val
    end
    (-infinity_value <=> obj).should == -1
  end

[seven1m]

But I guess a function such as rb_cmpint would be better, to be used generally where return values for use with cmp need converting (eg in Time as linked above).
However I'm not sure where that function would live in natalie, any thoughts welcome!

We have the junk drawer that is natalie.hpp/cpp -- you can stick a function in there and that would be fine. But perhaps it would make sense to put it on the Object class, kind of like we do for to_s and to_str etc.

[stevegeek]

@seven1m Do the latest changes look like they have gone in the right direction?

If so I could then refactor:

1. places where to_cmp_int is needed and currently implemented inline (eg in Time)
2. places where comparison errors are raised to instead use the method I added on Env (if its appropriate to add that method...)

[seven1m]

@stevegeek I had a minute to look at this today and decided to get a better handle on it myself. I made a commit to do pretty much the minimum to get the specs passing, and it looks good to me. I don't see the need for the to_cmp_int method right now, so I removed it. If you want to add your own tests to exercise it (we can add our own tests in the test/ directory), let's do it in another PR and add back the helper method you created!

Thanks so much for your work on this, I really appreciate it.

[stevegeek]

Ok sure up to you! The point of to_cmp_int it to ensure a common and Ruby compatable way of handling the return values that are passed to <=>, in all cases where such a thing would happen, eg in Time comparison, Array sort etc, however understand your position!

Ill see if I can get a set of specs added upstream to ruby/spec that make the need for such a method clearer and then if necessary come back to it!

[seven1m]

Yeah I think I see why we need the method, but we should have specs/tests driving that. Let's do it in another PR. I'm ok with us adding tests for it in the test/ directory.

[stevegeek]

Ok sure!