@jasnell wrote:

If we were to do this

I don't think there's any question we should do this. There is no reason for writeInt8() to return RangeError: Index out of range but writeFloatLE() to return RangeError: out of range index in the code below.

let buf = Buffer.alloc(9);
try {
  buf.writeInt8(1, -1); 
} catch (e) {
  console.log(e.toString()); // RangeError: Index out of range
}
try {
  buf.writeFloatLE(1, -1); 
} catch (e) {
  console.log(e.toString()); // RangeError: out of range index
}

@jasnell continued:

...I'd prefer to make it part of the migration to using internal/errors.js

Would you be OK with that being a subsequent PR? I'd prefer we separate out changes to error messages from migrations to internal/error.js rather than bundling those adjacent-but-not-integrally-related things. I can make sure that second PR happens immediately after this one lands if that affects your spider-sense on this one.

As long as the two land closely together, or perhaps even as two commits in the same PR, that's fine.

I will clear my "changes requested" review once I get a chance to review the other PR.

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

@jasnell It looks like sometimes this RangeError is thrown in JS-land (in which case we can migrate to the internal error system) and sometimes it is thrown by C++ (in which case we cannot, right? Maybe I'm wrong?).

For the sake of consistency, I wonder if it makes sense to not migrate the JS-land errors to the new system until we can do errors thrown in C++ as well. This way, you don't get very dissimilar RangeError objects depending on whether you are trying to read a float or an int. What do you think?

We're always going to have at least some inconsistency given that errors thrown from V8 will not have this information and neither will userland code. I will work on providing an equivalent mechanism for the errors thrown from C/C++ but I think it makes sense (and is best) to incrementally move forward. If we try to hold off and do them all at once, we're going to end up with too much of a moving target (I've already tried it). I think userland will be willing to forgive some short term inconsistency if it means we're making progress towards something better.

CI again: https://ci.nodejs.org/job/node-test-pull-request/6497/

There was a force push right after last CI, so it didn't go through.

New CI: https://ci.nodejs.org/job/node-test-pull-request/6519/

Heh, seems like there is a merge conflict: https://ci.nodejs.org/job/node-test-commit/8037/console

@larissayvette can you git rebase master?

@TimothyGu alright

Seems to be working now: https://ci.nodejs.org/job/node-test-pull-request/6521/

Tests are passing (hooray!) but linter is failing because a number of lines in test-buffer-fill.js are longer than 80 characters.

Easiest solution is probably to change things like this:

assert.throws(() =>
             Buffer.allocUnsafe(8).fill('a', -1),
              /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);

...to something more like this:

assert.throws(
  () => Buffer.allocUnsafe(8).fill('a', -1),
  /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/
);

Is there a problem using common.expectsError()?

@jasnell when common.expectsError() is used it produces this error
'undefined === ERR_INDEX_OUT_OF_RANGE`

@Trott I think the lint is correct now

On closer inspection, @jasnell is right to suggest that the regular expression is unnecessary in most of the places it's used here. It only needs to be used in places where the throw might be C++ or JS. That's probably only going to happen in test-buffer-write-noassert and/or test-buffer-read.

Keep in mind also that eventually, even the C/C++ errors thrown by Node.js will include these codes and the augmented name. We'll still need to be careful about errors thrown by V8 or Chakra

@jasnell Two things: Can you give this another look since it has changed a bit? Also, are we not landing these new internal error things on master yet? What needs to happen before we can start doing that?

Oh, it looks like this needs to be updated for the new common.expectsError() signature.

Instead of common.expectsError(code, type, msg), it's now common.expectsError({code, type, message: msg}).

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

Rebased and force pushed.

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

Landed in 234353a.
Thanks for the contribution, @larissayvette! 🎉