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.

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/natalie/env.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions include/natalie/object.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,10 @@ class Object : public Cell {
StringObject *to_s(Env *env);
StringObject *to_str(Env *env);

// If the object value will be returned from `<=>` and used in a comparison,
// then this method converts it into an integer in the set {-1, 0, 1}.
// If the conversion is not possible, then an ArgumentError is raised.
int to_cmp_int(Env *env, Value, Value);
protected:
ClassObject *m_klass { nullptr };

Expand Down
46 changes: 44 additions & 2 deletions spec/core/float/comparison_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
13 changes: 13 additions & 0 deletions src/env.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
23 changes: 18 additions & 5 deletions src/float_object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,27 @@ 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())
return Value::integer(1);
else
Value lhs = this;

if (is_infinity()) {
if (rhs->is_integer() && rhs->as_integer()->is_bignum()) {
if (is_positive_infinity()) return Value::integer(1);
return Value::integer(-1);
}

auto is_infinite_symbol = "infinite?"_s;
if (rhs->respond_to(env, is_infinite_symbol)) {
auto rhs_infinite = rhs.send(env, is_infinite_symbol);
if (rhs_infinite->is_nil()) {
if (is_positive_infinity()) return Value::integer(1);
return Value::integer(-1);
}
auto rhs_infinite_int = rhs_infinite->to_cmp_int(env, lhs, rhs);
int rhs_cmp = is_positive_infinity() ? (rhs_infinite_int > 0 ? 0 : 1) : (rhs_infinite_int < 0 ? 0 : -1);
return Value::integer(rhs_cmp);
}
}

Value lhs = this;
if (!rhs->is_float()) {
auto coerced = Natalie::coerce(env, rhs, lhs);
lhs = coerced.first;
Expand Down
22 changes: 22 additions & 0 deletions src/object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1166,4 +1166,26 @@ StringObject *Object::to_str(Env *env) {
result->klass()->inspect_str());
}

int Object::to_cmp_int(Env *env, Value lhs, Value rhs) {
if (is_nil()) {
env->raise_not_comparable_error(lhs, rhs);
}

if (is_integer()) {
seven1m marked this conversation as resolved.
Show resolved Hide resolved
auto i = as_integer();
if (i->send(env, ">"_s, { Value::integer(0) })->is_truthy())
return 1;
if (i->send(env, "<"_s, { Value::integer(0) })->is_truthy())
return -1;
return 0;
}

if (send(env, ">"_s, { Value::integer(0) })->is_truthy())
return 1;
else if (send(env, "<"_s, { Value::integer(0) })->is_truthy())
return -1;

return 0;
}

}