buffer: slow buffer copy compatibility fix

[trevnorris]

Fix issue where SlowBuffers couldn't be passed as targets for fast
buffer copy().

Also included small cleanups:

• Included documentation on how non uint parameters are handled
• Changed generic Errors to specifics
• Implemented quicker cc checks for undefined parameters
• Simplified Buffer checks
• Fixed buffer copy unit tests

There are many fixes here for Buffer.copy() that, imho, needed to be done, but too many for the v0.8 branch. I'll backport the basic changes and make another pull request for that.

Fix for GH-4633

[trevnorris]

Just as a side note, the reason for the extensive changes to the unit tests are because each buffer had the same values at the same intervals. So even if the copy failed silently, the asserts would still have passed.

[indutny]

target.parent || target?

[trevnorris]

Yeah, that was a stupid one. Should have just been placed in the return statement. Done.

[bnoordhuis]

This should be several small (atomic) commits, not a single big one.

[trevnorris]

@bnoordhuis Can do. Should each of those be their own PR, or could they be
in included here?

[bnoordhuis]

Using this PR is fine.

[trevnorris]

I've tried to split each set of changes as logically as possible.

The first commit has the bare essentials necessary for fixing GH-4633. @TooTallNate requested the change be placed on the v0.8 branch, so I've made sure it can be cherry-pick'd without conflict.

[bnoordhuis]

Mostly LGTM but I don't know if I agree with changing everything that's not an unsigned int to 0, especially values < 0.

It a) sounds like a potential source of obscure bugs in user code, and b) it could conceivably be used for something useful, like specifying from-end-of-buffer offsets.

@isaacs, @TooTallNate: Thoughts?

[trevnorris]

@bnoordhuis actually that comment is incorrect. It sets all non-number-y values that may be less than zero to 0, for start and target_start. So it has more or less the same effect as the previous check, except that negative values are set to 0. Just for example:

!(1 > 0) === false
!('1' > 0) === false
!(5.1 > 0) === false
!('a' > 0) === true
!(NaN > 0) === true

And actually, it should be >=. I'll fix that.

As for end, it does basically the same thing except for values greater than this.length. Reasoning behind this one was the previous implementation automatically was set to this.length if end was "oob".

I agree it could be a way to obscure bugs, but don't feel more than it did before. Now the "from-end-of-buffer offsets" is a good point. Though currently all values are ->Uint32Value(), which is a trivial fix, but would also need to be changed. If that's an implementation consideration then I'll put those checks in.

[trevnorris]

ok. took half a dozen rebases, but think it's taken care of.

[bnoordhuis]

LGTM. Maybe check with the others if we want to leave room in the API for negative offsets. I like the idea but maybe there are good reasons not to.

[trevnorris]

@bnoordhuis awesome. so, fixed the space issue I forgot. rebased off most recent master and @isaacs said no go on the negative offsets. think we're good. thanks a lot.

Update: ah crap. changed the commit msg and the code comments, but forgot to change the api. give me 5 to fix it.

[trevnorris]

@bnoordhuis ok. now I swear it's complete. updated the commit msg, api docs and code comment. also fixed a couple .equal() in the tests that were better set as .strictEqual().

[bnoordhuis]

Thanks Trevor, landed in 16bbecc, 49175e6 and cbe3941.

[bnoordhuis]

Small note: I wonder if calling IsUndefined() first is a minor deoptimization. Uint32Value() is only expensive when the value is not an SMI but that should be relatively rare.

[trevnorris]

@bnoordhuis You're correct. I was focused too much on another aspect. Here are my benchmarks that will help me explain:

                         before        after
fill - fast,fast,noArg:  143,277 µs    174,439 µs
fill - slow,slow,noArg:  308,928 µs    160,340 µs

So at the time I was focused on performance of calling copy against a SlowBuffer with no arguments. For some reason Uint32Value() is much slower when the argument is undefined. But since this is not the primary use case, feel free to revert that segment.

@TooTallNate depending on what @bnoordhuis wants to do here, I know you wanted to cherry-pick the commit to the v0.8 branch. Just a heads up that it was accepted.