Merge pull request #679 from stevegeek/fix_float_comparison_infinite

include/natalie/env.hpp

@@ -54,6 +54,7 @@ class Env : public Cell {
     [[noreturn]] void raise_errno();
     [[noreturn]] void raise_no_method_error(Object *, SymbolObject *, MethodMissingReason);
     [[noreturn]] void raise_name_error(SymbolObject *name, String);
+    [[noreturn]] void raise_not_comparable_error(Value lhs, Value rhs);
 
     template <typename... Args>
     [[noreturn]] void raise_name_error(SymbolObject *name, const char *format, Args... args) {

spec/core/float/comparison_spec.rb

@@ -4,7 +4,7 @@
   it "returns -1, 0, 1 when self is less than, equal, or greater than other" do
     (1.5 <=> 5).should == -1
     (2.45 <=> 2.45).should == 0
-    #((bignum_value*1.1) <=> bignum_value).should == 1
+    ((bignum_value*1.1) <=> bignum_value).should == 1
   end
 
   it "returns nil when either argument is NaN" do
@@ -14,6 +14,9 @@
 
   it "returns nil when the given argument is not a Float" do
     (1.0 <=> "1").should be_nil
+    (1.0 <=> "1".freeze).should be_nil
+    (1.0 <=> :one).should be_nil
+    (1.0 <=> true).should be_nil
   end
 
   it "compares using #coerce when argument is not a Float" do
@@ -48,7 +51,7 @@ def coerce(other)
 
   # The 4 tests below are taken from matz's revision 23730 for Ruby trunk
   #
-  it "returns 1 when self is Infinity and other is a Bignum" do
+  it "returns 1 when self is Infinity and other is an Integer" do
     (infinity_value <=> Float::MAX.to_i*2).should == 1
   end
 
@@ -63,4 +66,43 @@ def coerce(other)
   it "returns 1 when self is negative and other is -Infinity" do
     (-Float::MAX.to_i*2 <=> -infinity_value).should == 1
   end
+
+  it "returns 0 when self is Infinity and other other is infinite?=1" do
+    obj = Object.new
+    def obj.infinite?
+      1
+    end
+    (infinity_value <=> obj).should == 0
+  end
+
+  it "returns 1 when self is Infinity and other is infinite?=-1" do
+    obj = Object.new
+    def obj.infinite?
+      -1
+    end
+    (infinity_value <=> obj).should == 1
+  end
+
+  it "returns 1 when self is Infinity and other is infinite?=nil (which means finite)" do
+    obj = Object.new
+    def obj.infinite?
+        nil
+    end
+    (infinity_value <=> obj).should == 1
+  end
+
+  it "returns 0 for -0.0 and 0.0" do
+    (-0.0 <=> 0.0).should == 0
+    (0.0 <=> -0.0).should == 0
+  end
+
+  it "returns 0 for -0.0 and 0" do
+    (-0.0 <=> 0).should == 0
+    (0 <=> -0.0).should == 0
+  end
+
+  it "returns 0 for 0.0 and 0" do
+    (0.0 <=> 0).should == 0
+    (0 <=> 0.0).should == 0
+  end
 end

src/env.cpp

@@ -132,6 +132,19 @@ void Env::raise_name_error(SymbolObject *name, const String message) {
     this->raise_exception(exception);
 }
 
+void Env::raise_not_comparable_error(Value lhs, Value rhs) {
+    String lhs_class = lhs->klass()->inspect_str();
+    String rhs_inspect;
+    if (rhs->is_integer() || rhs->is_float() || rhs->is_falsey()) {
+        rhs_inspect = rhs->inspect_str(this);
+    } else {
+        rhs_inspect = rhs->klass()->inspect_str();
+    }
+
+    String message = String::format("comparison of {} with {} failed", lhs_class, rhs_inspect);
+    raise("ArgumentError", message);
+}
+
 void Env::warn(String message) {
     Value _stderr = global_get("$stderr"_s);
     message = String::format("warning: {}", message);

src/float_object.cpp

@@ -136,14 +136,28 @@ Value FloatObject::to_s() const {
 }
 
 Value FloatObject::cmp(Env *env, Value rhs) {
-    if (is_infinity() && rhs->is_integer() && rhs->as_integer()->is_bignum()) {
-        if (is_positive_infinity())
+    Value lhs = this;
+
+    auto infinite_sym = "infinite?"_s;
+
+    auto call_is_infinite = [&](Value value) -> Value {
+        if (value->respond_to(env, infinite_sym))
+            return value->send(env, infinite_sym);
+        return NilObject::the();
+    };
+
+    if (is_positive_infinity()) {
+        if (call_is_infinite(rhs) == Value::integer(1))
+            return Value::integer(0);
+        else
             return Value::integer(1);
+    } else if (is_negative_infinity()) {
+        if (call_is_infinite(rhs) == Value::integer(-1))
+            return Value::integer(0);
         else
             return Value::integer(-1);
     }
 
-    Value lhs = this;
     if (!rhs->is_float()) {
         auto coerced = Natalie::coerce(env, rhs, lhs);
         lhs = coerced.first;