process: improve process.hrtime#10764
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
jasnell added the semver-major 
jasnell commented Jan 12, 2017
Unfortunately because of the change in error condition this has to be labeled as semver-major. |
lib/internal/process.js Outdated
addaleax commented Jan 12, 2017
I don’t think it has to be. As @joyeecheung mentioned in the PR description, this only adds a throw for situations that we explicitly declared to be undefined behavior in the docs. It’s probably best to consider it breaking, unless there’s a good reason not to do that. |
benchmark/process/bench-hrtime.js Outdated
There was a problem hiding this comment.
Can you please make these separate var statements?
joyeecheung commented Jan 13, 2017
joyeecheung commented Jan 14, 2017
@jasnell If you still think it is semver-major, I am OK with landing this as semver-major. |
jasnell commented Jan 15, 2017
I'm more comfortable landing it as semver-major |
addaleax commented Jan 15, 2017
CI: https://ci.nodejs.org/job/node-test-commit/7257/ (Started this without seeing that one had already been kicked off. Sorry!) |
joyeecheung commented Jan 17, 2017
Going to land this if there is nothing else to be addressed. @mscdex Does this LGTY? |
test/parallel/test-process-hrtime.js Outdated
There was a problem hiding this comment.
How about adding another test that has > 2 elements?
One nit, but I agree with @jasnell that I would lean more towards semver-major to be on the safe side. Otherwise LGTM. |
* Add benchmarks for diffing a previous result * Improvements to the documentation, including type annotation * Update the outdated comments in src/node.cc, improve comments in lib/internal/process.js * Check the argument is an Array Tuple with length 2
joyeecheungforce-pushed the improve-hrtime branch from adda2bc to 0a68dbeComparejoyeecheung commented Jan 22, 2017
@mscdex Updated, PTAL. |
* Add benchmarks for diffing a previous result * Improvements to the documentation, including type annotation * Update the outdated comments in src/node.cc, improve comments in lib/internal/process.js * Check the argument is an Array Tuple with length 2 PR-URL: #10764 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Brian White <[email protected]>
joyeecheung commented Jan 23, 2017
Landed in a647d82 |
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
process
This PR is split into two commits in caution of breaking change, can be squashed when identified not breaking. Non breaking:
Might be breaking: check the length of argument passed into
process.hrtime(). Passing a user-defined array is previously described as undefined behavior in the documentation.Benchmark results: