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

Guard with isIterationCall #2231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 36 additions & 10 deletions underscore.js
Expand Up @@ -136,15 +136,35 @@
};
};

// Helper to determine if index is within the bounds of an array.
var isIndex = function(index, length) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this helper necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, but there's a bit of duplication between isArrayLike and isIterationCall that'll get worse once I propose only considering ints and not floats as proper index.

return index >= 0 && index < length;
};

// Helper for collection methods to determine whether a collection
// should be iterated as an array or as an object
// should be iterated as an array or as an object.
// Related: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-tolength
// Avoids a very nasty iOS 8 JIT bug on ARM-64. #2094
var MAX_ARRAY_INDEX = Math.pow(2, 53) - 1;
var getLength = property('length');
var isArrayLike = function(collection) {
var length = getLength(collection);
return typeof length == 'number' && length >= 0 && length <= MAX_ARRAY_INDEX;
return typeof length == 'number' && isIndex(length, MAX_ARRAY_INDEX);
};

// Helper to determine if the arguments to a function are from
// an iteration of `each`, `map`, and friends.
var isIterationCall = function(value, index, collection) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a wrapper function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know,

_.max = gaurdIterationCall(function() {

});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd be nifty, but I think it'd slow down common usage (it'll require an #apply). And I don't think the other contributors want more wrappers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we could get away with a direct invocation as none of them require this context, but I hear you

if (!_.isObject(collection)) return false;
var contains = typeof index === 'number' ?
isArrayLike(collection) && isIndex(index, collection.length) :
index in collection;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we care about sparse array handling? (I.e. it won't be gaurded for sparse array indexes)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just not test index in collection

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we care about sparse array handling?

Sparse arrays will hit the isArrayLike(collection) && isIndex(index, collection.length) check.

I'd rather just not test index in collection

We have to for object iteration.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just not test index in collection

We have to for object iteration.

Whoops read the code too quickly

if (contains) {
var other = collection[index];
return value === value ? value === other : other !== other;
}

return false;
};

// Collection Functions
Expand Down Expand Up @@ -271,7 +291,9 @@
// Aliased as `includes` and `include`.
_.contains = _.includes = _.include = function(obj, item, fromIndex, guard) {
if (!isArrayLike(obj)) obj = _.values(obj);
if (typeof fromIndex != 'number' || guard) fromIndex = 0;
if (typeof fromIndex != 'number' || (guard && isIterationCall(item, fromIndex, guard))) {
fromIndex = 0;
}
return _.indexOf(obj, item, fromIndex) >= 0;
};

Expand Down Expand Up @@ -305,7 +327,8 @@
_.max = function(obj, iteratee, context) {
var result = -Infinity, lastComputed = -Infinity,
value, computed;
if (iteratee == null || (typeof iteratee == 'number' && typeof obj[0] != 'object') && obj != null) {
if (context && isIterationCall(obj, iteratee, context)) iteratee = null;
if (iteratee == null) {
obj = isArrayLike(obj) ? obj : _.values(obj);
for (var i = 0, length = obj.length; i < length; i++) {
value = obj[i];
Expand All @@ -330,7 +353,8 @@
_.min = function(obj, iteratee, context) {
var result = Infinity, lastComputed = Infinity,
value, computed;
if (iteratee == null || (typeof iteratee == 'number' && typeof obj[0] != 'object') && obj != null) {
if (context && isIterationCall(obj, iteratee, context)) iteratee = null;
if (iteratee == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding a test for this change

obj = isArrayLike(obj) ? obj : _.values(obj);
for (var i = 0, length = obj.length; i < length; i++) {
value = obj[i];
Expand Down Expand Up @@ -361,7 +385,7 @@
// If **n** is not specified, returns a single random element.
// The internal `guard` argument allows it to work with `map`.
_.sample = function(obj, n, guard) {
if (n == null || guard) {
if (n == null || (guard && isIterationCall(obj, n, guard))) {
if (!isArrayLike(obj)) obj = _.values(obj);
return obj[_.random(obj.length - 1)];
}
Expand Down Expand Up @@ -458,30 +482,32 @@
// allows it to work with `_.map`.
_.first = _.head = _.take = function(array, n, guard) {
if (array == null) return void 0;
if (n == null || guard) return array[0];
if (n == null || (guard && isIterationCall(array, n, guard))) return array[0];
return _.initial(array, array.length - n);
};

// Returns everything but the last entry of the array. Especially useful on
// the arguments object. Passing **n** will return all the values in
// the array, excluding the last N.
_.initial = function(array, n, guard) {
return slice.call(array, 0, Math.max(0, array.length - (n == null || guard ? 1 : n)));
if (n == null || (guard && isIterationCall(array, n, guard))) n = 1;
return slice.call(array, 0, Math.max(0, array.length - n));
};

// Get the last element of an array. Passing **n** will return the last N
// values in the array.
_.last = function(array, n, guard) {
if (array == null) return void 0;
if (n == null || guard) return array[array.length - 1];
if (n == null || (guard && isIterationCall(array, n, guard))) return array[array.length - 1];
return _.rest(array, Math.max(0, array.length - n));
};

// Returns everything but the first entry of the array. Aliased as `tail` and `drop`.
// Especially useful on the arguments object. Passing an **n** will return
// the rest N values in the array.
_.rest = _.tail = _.drop = function(array, n, guard) {
return slice.call(array, n == null || guard ? 1 : n);
if (n == null || (guard && isIterationCall(array, n, guard))) n = 1;
return slice.call(array, n);
};

// Trim out all falsy values from an array.
Expand Down