From 9fba086e371e79db5a483b7154a717a9832b5fcf Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Fri, 24 Jan 2020 22:54:19 +0100 Subject: [PATCH 1/2] Fix Array keep_if, delete_if, reject!, select! Match CRuby behavior when exceptions are raised in the block and about when the array is updated. --- UNRELEASED.md | 4 +++ opal/corelib/array.rb | 60 ++++++++++++++++++++------------------ spec/filters/bugs/array.rb | 4 --- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index f941b80bb2..022bb849dc 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -25,6 +25,10 @@ Whitespace conventions: ### Fixed +- Array#delete_if (#2069) +- Array#keep_if (#2069) +- Array#reject! (#2069) +- Array#select! (#2069) - Struct#dup (#1995) - Integer#gcdlcm (#1972) - Enumerable#to_h (#1979) diff --git a/opal/corelib/array.rb b/opal/corelib/array.rb index 509a09332f..7a1bdfc7ad 100644 --- a/opal/corelib/array.rb +++ b/opal/corelib/array.rb @@ -1,3 +1,5 @@ +# helpers: truthy, falsy + require 'corelib/enumerable' require 'corelib/numeric' @@ -15,6 +17,34 @@ class Array < `Array` return klass.$allocate().$replace(#{`obj`.to_a}); } } + + // A helper for keep_if and delete_if, filter is either Opal.truthy + // or Opal.falsy. + function filterIf(self, filter, block) { + var value, raised = null, updated = new Array(self.length); + + for (var i = 0, i2 = 0, length = self.length; i < length; i++) { + if (!raised) { + try { + value = Opal.yield1(block, self[i]) + } catch(error) { + raised = error; + } + } + + if (raised || filter(value)) { + updated[i2] = self[i] + i2 += 1; + } + } + + if (i2 !== i) { + self.splice.apply(self, [0, updated.length].concat(updated)); + self.splice(i2, updated.length); + } + + if (raised) throw raised; + } } def self.[](*objects) @@ -826,20 +856,7 @@ def delete_at(index) def delete_if(&block) return enum_for(:delete_if) { size } unless block_given? - - %x{ - for (var i = 0, length = self.length, value; i < length; i++) { - value = block(self[i]); - - if (value !== false && value !== nil) { - self.splice(i, 1); - - length--; - i--; - } - } - } - + %x{filterIf(self, $falsy, block)} self end @@ -1355,20 +1372,7 @@ def join(sep = nil) def keep_if(&block) return enum_for(:keep_if) { size } unless block_given? - - %x{ - for (var i = 0, length = self.length, value; i < length; i++) { - value = block(self[i]); - - if (value === false || value === nil) { - self.splice(i, 1); - - length--; - i--; - } - } - } - + %x{filterIf(self, $truthy, block)} self end diff --git a/spec/filters/bugs/array.rb b/spec/filters/bugs/array.rb index e060004624..05a1b9b961 100644 --- a/spec/filters/bugs/array.rb +++ b/spec/filters/bugs/array.rb @@ -2,7 +2,6 @@ opal_filter "Array" do fails "Array#== compares with an equivalent Array-like object using #to_ary" # Expected false to be true fails "Array#== returns true for [NaN] == [NaN] because Array#== first checks with #equal? and NaN.equal?(NaN) is true" # Expected [NaN] to equal [NaN] - fails "Array#delete_if updates the receiver after all blocks" fails "Array#each does not yield elements deleted from the end of the array" # Expected [2, 3, nil] to equal [2, 3] fails "Array#each yields elements added to the end of the array by the block" # Expected [2] to equal [2, 0, 0] fails "Array#flatten does not call #to_ary on elements beyond the given level" @@ -10,13 +9,10 @@ fails "Array#flatten with a non-Array object in the Array calls #method_missing if defined" fails "Array#inspect does not call #to_str on the object returned from #to_s when it is not a String" # Exception: Cannot convert object to primitive value fails "Array#join raises a NoMethodError if an element does not respond to #to_str, #to_ary, or #to_s" - fails "Array#keep_if updates the receiver after all blocks" fails "Array#partition returns in the left array values for which the block evaluates to true" fails "Array#rassoc calls elem == obj on the second element of each contained array" fails "Array#rassoc does not check the last element in each contained but specifically the second" # Expected [1, "foobar", #] to equal [2, #, 1] - fails "Array#reject! updates the receiver after all blocks" fails "Array#select returns a new array of elements for which block is true" - fails "Array#select! updates the receiver after all blocks" fails "Array#to_s does not call #to_str on the object returned from #to_s when it is not a String" # Exception: Cannot convert object to primitive value fails "Array#uniq! properly handles recursive arrays" fails "Array#zip fills nil when the given enumerator is shorter than self" # LocalJumpError: no block given From 1d6d26242b22c1b0efa0117f9e7e1dc05b067595 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Mon, 27 Jan 2020 23:57:39 +0100 Subject: [PATCH 2/2] Move some Opal helpers to cached Should give better performance and smaller code size. --- opal/corelib/array.rb | 66 +++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/opal/corelib/array.rb b/opal/corelib/array.rb index 7a1bdfc7ad..c11c5fbf5e 100644 --- a/opal/corelib/array.rb +++ b/opal/corelib/array.rb @@ -1,4 +1,4 @@ -# helpers: truthy, falsy +# helpers: truthy, falsy, hash_ids, yield1, hash_get, hash_put, hash_delete require 'corelib/enumerable' require 'corelib/numeric' @@ -26,7 +26,7 @@ class Array < `Array` for (var i = 0, i2 = 0, length = self.length; i < length; i++) { if (!raised) { try { - value = Opal.yield1(block, self[i]) + value = $yield1(block, self[i]) } catch(error) { raised = error; } @@ -120,12 +120,12 @@ def &(other) var result = [], hash = #{{}}, i, length, item; for (i = 0, length = other.length; i < length; i++) { - Opal.hash_put(hash, other[i], true); + $hash_put(hash, other[i], true); } for (i = 0, length = self.length; i < length; i++) { item = self[i]; - if (Opal.hash_delete(hash, item) !== undefined) { + if ($hash_delete(hash, item) !== undefined) { result.push(item); } } @@ -145,11 +145,11 @@ def |(other) var hash = #{{}}, i, length, item; for (i = 0, length = self.length; i < length; i++) { - Opal.hash_put(hash, self[i], true); + $hash_put(hash, self[i], true); } for (i = 0, length = other.length; i < length; i++) { - Opal.hash_put(hash, other[i], true); + $hash_put(hash, other[i], true); } return hash.$keys(); @@ -201,12 +201,12 @@ def -(other) var result = [], hash = #{{}}, i, length, item; for (i = 0, length = other.length; i < length; i++) { - Opal.hash_put(hash, other[i], true); + $hash_put(hash, other[i], true); } for (i = 0, length = self.length; i < length; i++) { item = self[i]; - if (Opal.hash_get(hash, item) === undefined) { + if ($hash_get(hash, item) === undefined) { result.push(item); } } @@ -537,7 +537,7 @@ def bsearch_index(&block) while (min < max) { mid = min + Math.floor((max - min) / 2); val = self[mid]; - ret = Opal.yield1(block, val); + ret = $yield1(block, val); if (ret === true) { satisfied = mid; @@ -595,7 +595,7 @@ def cycle(n = nil, &block) if (n === nil) { while (true) { for (i = 0, length = self.length; i < length; i++) { - value = Opal.yield1(block, self[i]); + value = $yield1(block, self[i]); } } } @@ -607,7 +607,7 @@ def cycle(n = nil, &block) while (n > 0) { for (i = 0, length = self.length; i < length; i++) { - value = Opal.yield1(block, self[i]); + value = $yield1(block, self[i]); } n--; @@ -643,7 +643,7 @@ def collect(&block) var result = []; for (var i = 0, length = self.length; i < length; i++) { - var value = Opal.yield1(block, self[i]); + var value = $yield1(block, self[i]); result.push(value); } @@ -656,7 +656,7 @@ def collect!(&block) %x{ for (var i = 0, length = self.length; i < length; i++) { - var value = Opal.yield1(block, self[i]); + var value = $yield1(block, self[i]); self[i] = value; } } @@ -904,7 +904,7 @@ def each(&block) %x{ for (var i = 0, length = self.length; i < length; i++) { - var value = Opal.yield1(block, self[i]); + var value = $yield1(block, self[i]); } } @@ -916,7 +916,7 @@ def each_index(&block) %x{ for (var i = 0, length = self.length; i < length; i++) { - var value = Opal.yield1(block, i); + var value = $yield1(block, i); } } @@ -1183,29 +1183,29 @@ def flatten!(level = undefined) def hash %x{ - var top = (Opal.hash_ids === undefined), + var top = ($hash_ids === undefined), result = ['A'], hash_id = self.$object_id(), item, i, key; try { if (top) { - Opal.hash_ids = Object.create(null); + $hash_ids = Object.create(null); } // return early for recursive structures - if (Opal.hash_ids[hash_id]) { + if ($hash_ids[hash_id]) { return 'self'; } - for (key in Opal.hash_ids) { - item = Opal.hash_ids[key]; + for (key in $hash_ids) { + item = $hash_ids[key]; if (#{eql?(`item`)}) { return 'self'; } } - Opal.hash_ids[hash_id] = self; + $hash_ids[hash_id] = self; for (i = 0; i < self.length; i++) { item = self[i]; @@ -1215,7 +1215,7 @@ def hash return result.join(','); } finally { if (top) { - Opal.hash_ids = undefined; + $hash_ids = undefined; } } } @@ -1475,7 +1475,7 @@ def permutation(num = undefined, &block) for (var j = 0; j < perm.length; j++) { output.push(self[perm[j]]); } - Opal.yield1(blk, output); + $yield1(blk, output); } } } @@ -1881,7 +1881,7 @@ def select(&block) for (var i = 0, length = self.length, item, value; i < length; i++) { item = self[i]; - value = Opal.yield1(block, item); + value = $yield1(block, item); if (Opal.truthy(value)) { result.push(item); @@ -2151,7 +2151,7 @@ def to_h } key = ary[0]; val = ary[1]; - Opal.hash_put(hash, key, val); + $hash_put(hash, key, val); } return hash; @@ -2195,17 +2195,17 @@ def uniq(&block) if (block === nil) { for (i = 0, length = self.length; i < length; i++) { item = self[i]; - if (Opal.hash_get(hash, item) === undefined) { - Opal.hash_put(hash, item, item); + if ($hash_get(hash, item) === undefined) { + $hash_put(hash, item, item); } } } else { for (i = 0, length = self.length; i < length; i++) { item = self[i]; - key = Opal.yield1(block, item); - if (Opal.hash_get(hash, key) === undefined) { - Opal.hash_put(hash, key, item); + key = $yield1(block, item); + if ($hash_get(hash, key) === undefined) { + $hash_put(hash, key, item); } } } @@ -2220,10 +2220,10 @@ def uniq!(&block) for (i = 0, length = original_length; i < length; i++) { item = self[i]; - key = (block === nil ? item : Opal.yield1(block, item)); + key = (block === nil ? item : $yield1(block, item)); - if (Opal.hash_get(hash, key) === undefined) { - Opal.hash_put(hash, key, item); + if ($hash_get(hash, key) === undefined) { + $hash_put(hash, key, item); continue; }