[GR-23533] Refactor integer conversion nodes.

PullRequest: truffleruby/1613

Author: Nicolas Laurent

spec/mspec/lib/mspec/helpers/numeric.rb

@@ -12,6 +12,14 @@ def bignum_value(plus = 0)
   0x8000_0000_0000_0000 + plus
 end
 
+def max_long
+  2**(0.size * 8 - 1) - 1
+end
+
+def min_long
+  -(2**(0.size * 8 - 1))
+end
+
 # This is a bit hairy, but we need to be able to write specs that cover the
 # boundary between Fixnum and Bignum for operations like Fixnum#<<. Since
 # this boundary is implementation-dependent, we use these helpers to write

spec/ruby/core/array/shared/slice.rb

@@ -466,6 +466,12 @@ def to.to_int() -2 end
 
     obj = 8e19
     -> { array.send(@method, obj) }.should raise_error(RangeError)
+
+    # boundary value when longs are 64 bits
+    -> { array.send(@method, 2.0**63) }.should raise_error(RangeError)
+
+    # just under the boundary value when longs are 64 bits
+    array.send(@method, max_long.to_f.prev_float).should == nil
   end
 
   it "raises a RangeError when the length is out of range of Fixnum" do

spec/ruby/core/integer/left_shift_spec.rb

@@ -71,8 +71,11 @@
     it "calls #to_int to convert the argument to an Integer" do
       obj = mock("4")
       obj.should_receive(:to_int).and_return(4)
-
       (3 << obj).should == 48
+
+      obj = mock("to_int_neg_bignum")
+      obj.should_receive(:to_int).and_return(-bignum_value)
+      (3 << obj).should == 0
     end
 
     it "raises a TypeError when #to_int does not return an Integer" do

spec/ruby/core/integer/right_shift_spec.rb

@@ -71,8 +71,11 @@
     it "calls #to_int to convert the argument to an Integer" do
       obj = mock("2")
       obj.should_receive(:to_int).and_return(2)
-
       (8 >> obj).should == 2
+
+      obj = mock("to_int_bignum")
+      obj.should_receive(:to_int).and_return(bignum_value)
+      (8 >> obj).should == 0
     end
 
     it "raises a TypeError when #to_int does not return an Integer" do

spec/ruby/shared/string/times.rb

@@ -20,10 +20,16 @@ class MyString < String; end
   it "raises an ArgumentError when given integer is negative" do
     -> { @object.call("cool", -3)    }.should raise_error(ArgumentError)
     -> { @object.call("cool", -3.14) }.should raise_error(ArgumentError)
+    -> { @object.call("cool", min_long) }.should raise_error(ArgumentError)
   end
 
   it "raises a RangeError when given integer is a Bignum" do
     -> { @object.call("cool", 999999999999999999999) }.should raise_error(RangeError)
+    -> { @object.call("", 999999999999999999999) }.should raise_error(RangeError)
+  end
+
+  it "works with huge long values when string is empty" do
+    @object.call("", max_long).should == ""
   end
 
   it "returns subclass instances" do

src/main/java/org/truffleruby/core/array/ArrayNodes.java

@@ -33,10 +33,10 @@
 import org.truffleruby.core.array.library.NativeArrayStorage;
 import org.truffleruby.core.cast.BooleanCastNode;
 import org.truffleruby.core.cast.CmpIntNode;
+import org.truffleruby.core.cast.ToIntNode;
+import org.truffleruby.core.cast.ToLongNode;
 import org.truffleruby.core.cast.ToAryNode;
 import org.truffleruby.core.cast.ToAryNodeGen;
-import org.truffleruby.core.cast.ToIntNode;
-import org.truffleruby.core.cast.ToIntNodeGen;
 import org.truffleruby.core.cast.ToStrNodeGen;
 import org.truffleruby.core.format.BytesResult;
 import org.truffleruby.core.format.FormatExceptionTranslator;
@@ -281,7 +281,7 @@ public abstract static class AtNode extends CoreMethodNode {
 
         @CreateCast("index")
         protected RubyNode coerceOtherToInt(RubyNode index) {
-            return FixnumLowerNodeGen.create(ToIntNodeGen.create(index));
+            return FixnumLowerNodeGen.create(ToIntNode.create(index));
         }
 
         @Specialization
@@ -569,7 +569,7 @@ public abstract static class DeleteAtNode extends CoreMethodNode {
 
         @CreateCast("index")
         protected RubyNode coerceOtherToInt(RubyNode index) {
-            return ToIntNodeGen.create(index);
+            return ToIntNode.create(index);
         }
 
         @Specialization(
@@ -835,39 +835,25 @@ public abstract static class HashNode extends ArrayCoreMethodNode {
 
         private static final int CLASS_SALT = 42753062; // random number, stops hashes for similar values but different classes being the same, static because we want deterministic hashes
 
-        @Child private ToIntNode toIntNode;
-
         @Specialization(limit = "storageStrategyLimit()")
         protected long hash(VirtualFrame frame, DynamicObject array,
                 @CachedLibrary("getStore(array)") ArrayStoreLibrary stores,
-                @Cached("createPrivate()") CallDispatchHeadNode toHashNode) {
+                @Cached("createPrivate()") CallDispatchHeadNode toHashNode,
+                @Cached ToLongNode toLongNode) {
             final int size = Layouts.ARRAY.getSize(array);
             long h = getContext().getHashing(this).start(size);
             h = Hashing.update(h, CLASS_SALT);
             final Object store = Layouts.ARRAY.getStore(array);
 
             for (int n = 0; n < size; n++) {
                 final Object value = stores.read(store, n);
-                final long valueHash = toLong(toHashNode.call(value, "hash"));
+                final long valueHash = toLongNode.execute(toHashNode.call(value, "hash"));
                 h = Hashing.update(h, valueHash);
             }
 
             return Hashing.end(h);
         }
 
-        private long toLong(Object indexObject) {
-            if (toIntNode == null) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                toIntNode = insert(ToIntNode.create());
-            }
-            final Object result = toIntNode.executeIntOrLong(indexObject);
-            if (result instanceof Integer) {
-                return (int) result;
-            } else {
-                return (long) result;
-            }
-        }
-
     }
 
     @CoreMethod(names = "include?", required = 1)
@@ -1091,7 +1077,7 @@ protected int toInt(Object value) {
                 CompilerDirectives.transferToInterpreterAndInvalidate();
                 toIntNode = insert(ToIntNode.create());
             }
-            return toIntNode.doInt(value);
+            return toIntNode.execute(value);
         }
 
     }
@@ -1437,19 +1423,12 @@ protected int getCacheLimit() {
     @ReportPolymorphism
     public abstract static class PopNode extends ArrayCoreMethodNode {
 
-        @Child private ToIntNode toIntNode;
-        @Child private ArrayPopOneNode popOneNode;
-
         public abstract Object executePop(DynamicObject array, Object n);
 
         @Specialization
         @ReportPolymorphism.Exclude
-        protected Object pop(DynamicObject array, NotProvided n) {
-            if (popOneNode == null) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                popOneNode = insert(ArrayPopOneNodeGen.create());
-            }
-
+        protected Object pop(DynamicObject array, NotProvided n,
+                @Cached ArrayPopOneNode popOneNode) {
             return popOneNode.executePopOne(array);
         }
 
@@ -1513,16 +1492,9 @@ protected Object popNotEmptyUnsharedStorage(DynamicObject array, int n,
         }
 
         @Specialization(guards = { "wasProvided(n)", "!isInteger(n)", "!isLong(n)" })
-        protected Object popNToInt(DynamicObject array, Object n) {
-            return executePop(array, toInt(n));
-        }
-
-        private int toInt(Object indexObject) {
-            if (toIntNode == null) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                toIntNode = insert(ToIntNode.create());
-            }
-            return toIntNode.doInt(indexObject);
+        protected Object popNToInt(DynamicObject array, Object n,
+                @Cached ToIntNode toIntNode) {
+            return executePop(array, toIntNode.execute(n));
         }
 
     }
@@ -1926,16 +1898,9 @@ protected Object shiftMany(DynamicObject array, int n,
         }
 
         @Specialization(guards = { "wasProvided(n)", "!isInteger(n)", "!isLong(n)" })
-        protected Object shiftNToInt(DynamicObject array, Object n) {
-            return executeShift(array, toInt(n));
-        }
-
-        private int toInt(Object indexObject) {
-            if (toIntNode == null) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                toIntNode = insert(ToIntNode.create());
-            }
-            return toIntNode.doInt(indexObject);
+        protected Object shiftNToInt(DynamicObject array, Object n,
+                @Cached ToIntNode toIntNode) {
+            return executeShift(array, toIntNode.execute(n));
         }
 
     }

src/main/java/org/truffleruby/core/cast/IntegerCastNode.java

@@ -15,14 +15,13 @@
 import org.truffleruby.language.RubyGuards;
 import org.truffleruby.language.control.RaiseException;
 
-import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.CachedContext;
 import com.oracle.truffle.api.dsl.GenerateUncached;
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.object.DynamicObject;
+import org.truffleruby.utils.Utils;
 
-/** Casts a value into an int. */
+/** See {@link ToIntNode} for a comparison of different integer conversion nodes. */
 @GenerateUncached
 @ImportStatic(RubyGuards.class)
 public abstract class IntegerCastNode extends RubyBaseNode {
@@ -46,18 +45,16 @@ protected int doLong(long value) {
     @Specialization(guards = "!fitsInInteger(value)")
     protected int doLongToBig(long value,
             @CachedContext(RubyLanguage.class) RubyContext context) {
-        throw new RaiseException(context, notAFixnum(context, value));
+        throw new RaiseException(
+                context,
+                context.getCoreExceptions().rangeError("long too big to convert into `int'", this));
     }
 
-    @Specialization(guards = { "!isBasicInteger(value)" })
+    @Specialization(guards = "!isBasicInteger(value)")
     protected int doBasicObject(Object value,
             @CachedContext(RubyLanguage.class) RubyContext context) {
-        throw new RaiseException(context, notAFixnum(context, value));
+        throw new RaiseException(
+                context,
+                context.getCoreExceptions().typeErrorIsNotA(Utils.toString(value), "Integer (fitting in int)", this));
     }
-
-    @TruffleBoundary
-    private DynamicObject notAFixnum(RubyContext context, Object object) {
-        return context.getCoreExceptions().typeErrorIsNotA(object.toString(), "Fixnum (fitting in int)", this);
-    }
-
 }

src/main/java/org/truffleruby/core/cast/LongCastNode.java

@@ -20,9 +20,8 @@
 import com.oracle.truffle.api.dsl.GenerateUncached;
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.object.DynamicObject;
 
-/** Casts a value into a long. */
+/** See {@link ToIntNode} for a comparison of different integer conversion nodes. */
 @GenerateUncached
 @ImportStatic(RubyGuards.class)
 public abstract class LongCastNode extends RubyBaseNode {
@@ -43,18 +42,12 @@ protected long doLong(long value) {
         return value;
     }
 
+    @TruffleBoundary
     @Specialization(guards = "!isBasicInteger(value)")
     protected long doBasicObject(Object value,
             @CachedContext(RubyLanguage.class) RubyContext context) {
-        throw new RaiseException(context, notAFixnum(context, value));
+        throw new RaiseException(
+                context,
+                context.getCoreExceptions().typeErrorIsNotA(value.toString(), "Integer (fitting in long)", this));
     }
-
-    @TruffleBoundary
-    private DynamicObject notAFixnum(RubyContext context, Object object) {
-        return context.getCoreExceptions().typeErrorIsNotA(
-                object.toString(),
-                "Fixnum (fitting in long)",
-                this);
-    }
-
 }