I would change the commit message to start with This patch fixes ... then place Following the discussion nodejs#2668 at the bottom as metadata. e.g.

Ref: https://github.com/nodejs/node/issues/2668

@trevnorris Updated for NaN.

Great. If CI is happy then LGTM.

This is a semver-major, so will go in the v5 release.

CI Run: https://ci.nodejs.org/job/node-test-pull-request/340/

I am not able to test this now.
What happens on large positive / large negative numbers? Could you include those in the testcase?

@ChALkeR They'll wrap. Same as they always have.

@trevnorris Ah. So that is out of scope for this commit.

This commit now converts negative and NaN values of end to this.length. This looks mostly compatible to what there was before this patch.

LGTM, though such behavior is a bit strange.

Judging from the surrounding code, it looks like negative values of end were meant to return an empty string. @trevnorris, what do you think? Though that will be a breaking change, while this one isn't.

@ChALkeR Good catch. You are correct.

This PR is a great fix for existing undefined behavior, and making end equal to the length if less than zero seems counter intuitive. Personal opinion, it should be set equal to zero.

@trevnorris @ChALkeR Made sure that the negative value for end is taken as 0.

I'd still recommend putting the reference issue as I posted above, but if CI is good then LGTM.

CI Run before landing: https://ci.nodejs.org/job/node-test-pull-request/364/

@trevnorris I'll fix that while landing. @ChALkeR LGTY?

LGTM code-wise.

Only a minor notice that I would re-order the lines and group together the checks based on the variable that they are related to. Something like this:

  start = +start;
  if (!Number.isInteger(start)) {
    start = start >>> 0;
  }
  if (start < 0) start = 0;

  if (end === undefined || end === Infinity || end !== end) {
    end = this.length;
  }
  end = +end < 0 ? 0 : end >>> 0;
  if (end > this.length) end = this.length;
  if (end <= start) return '';

  if (!encoding) encoding = 'utf8';
  var loweredCase = false;

But that is not required and not everyone might agree with this change, I guess.
@trevnorris ?

Eh? this is semver-major? If yes, should the start/end arguments be fixed here completely so they don't get wrapped? @trevnorris, what do you think?

Here is what I mean:

  start = +start;
  if (start >= this.length) return '';
  start = (start > 0) ? start >>> 0 : 0;

  if (end === undefined || end === Infinity || end !== end) {
    end = this.length;
  } else {
    end = +end;
    if (end <= start) return '';
    end = (end <= this.length) ? end >>> 0 : this.length;
  }

  if (!encoding) encoding = 'utf8';
  var loweredCase = false;

Was going to save some of this for later, but while we're here.

I'd say while your here we should make sure parsing is consistent. For example end is first uint32 coerced in JS then int32 coerced in C++. I may suggest we change ParseArrayIndex() in node_internals.h to use Uint32Value() instead of Int32Value(). Then make sure values in JS are a uint32.

This would mean the values will go through n >>> 0 before being passed to C++.

Open to ideas on how we handle out of range values. I suppose we'll floor any non-integers that are in the uint32 range with the >>> operation.

Yeah, the internals should have unsigned 32-bit numbers, and the js side should validate the indicies for them to be in the allowable range (from 0 to this.length). That's exactly what I proposed on the js side in the above message.

Atm, this PR converts the start to start >>> 0 without first checking that it's in between 0 and this.length, so 4294967296 would become 0, which is unexpected and should probably be fixed here while we are on a semver-major change.

okay. so how about this:

if (start >= this.length || end <= 0)
  return '';

if (start < 0 || start >>> 0 != parseInt(+start))
  start = 0;
else
  start >>>= 0;

if (end > this.length || end >>> 0 != parseInt(+end))
  end = this.length;
else
  end >>>= 0;

if (end <= start)
  return '';

That should handle all the cases discussed on IRC.

UPDATE: Based on IRC discussion.

@trevnorris That code LGTM.

@ChALkeR awesome. Great team work everyone putting together these coercion checks.

@ChALkeR @trevnorris I updated the PR as per our discussion. PTAL.

two comments, and amazing job on the tests.

@thefourtheye want to move forward with this?

@thefourtheye I can take this over and move it forward if you're ok with that.

@matthewloring please go ahead. Thanks for taking it up :)