doc: update process.hrtime docs to include optional parameter #6585
Conversation
| @@ -745,7 +745,7 @@ process.exit(1); | |||
| The shell that executed Node.js should see the exit code as `1`. | |||
|
|
|||
| It is important to note that calling `process.exit()` will force the process to | |||
| exit as quickly as possible *even if there are still asynchronous operations | |||
There was a problem hiding this comment.
Yikes. Looks like my text editor stripped trailing spaces; I'm guessing they're important, so I'll fix the commit real quick.
There was a problem hiding this comment.
I assume you they have been inserted erroneously. Since other lines below have none I would prefer to have it "fixed" now. @jasnell do you have an opinion?
There was a problem hiding this comment.
well... two trailing spaces in markdown means line break. though it's one of the most stupid things I've ever heard of, so don't worry about it.
doug-wade
force-pushed
the
fix-6571-process-hrtime-docs
branch
from
773e99b
to
bf51815
Compare
|
LGTM thank you! |
|
@nodejs/documentation |
|
Good one! LGTM and thanks. |
|
Please note the discussion here: #4757 (comment) If we document it that way, it could become ambiguous of what should happen in corner cases, i.e. when the time is not a result of a previous call to /cc @trevnorris, @bnoordhuis |
|
@ChALkeR your suggestion is to make it more explicit that you could get negative times, or other strange behaviors, if you try to construct a time array by hand? |
|
If it's clearly documented what the array should be (a return from a previous |
doug-wade
force-pushed
the
fix-6571-process-hrtime-docs
branch
from
bf51815
to
092da04
Compare
|
@trevnorris ... any further thoughts on this? |
|
@jasnell thanks for the heads' up. I'll be more proactive about mentioning reviewers in the future. |
|
No worries :-) |
|
@jasnell basically that if the array has a negative value, etc., then the returning array will have garbage numbers in it. we only make a guarantee about what the return value should be if the input values match what we expect. anything else is UB. so we don't have to put in a bunch of pointless type/range checks. |
|
@trevnorris I added the following verbiage in addition to the original specification Does that sufficiently address your concern? |
|
@doug-wade That reads excellent. Allows us to keep the existing implementation. |
|
@jasnell anything else I need to do before I merge? |
| related to the time of day and therefore not subject to clock drift. The | ||
| primary use is for measuring performance between intervals. | ||
| tuple Array. `time` is an optional parameter that must be the result of a | ||
| previous `process.hrtime` call (and therefore, a real time in a |
There was a problem hiding this comment.
process.htime() (we typically add the () when mentioning function names)
|
One small additional nit but otherwise LGTM |
|
Still LGTM w/ @jasnell nits addressed and one extra comment. |
Adds more explicit documentation for the single optional parameter to process.hrtime to the process docs.
doug-wade
force-pushed
the
fix-6571-process-hrtime-docs
branch
from
092da04
to
6c221dc
Compare
|
@jasnell fixed up the missing parens nit |
|
LGTM! |
|
LGTM |
|
Thanks for the changes. LGTM |
jasnell
changed the title
Adds more explicit documentation for the single optional parameter to process.hrtime to the process docs. PR-URL: #6585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
|
Landed in e916218 |
|
I just noticed this commit contains a reference to |
Adds more explicit documentation for the single optional parameter to process.hrtime to the process docs. PR-URL: nodejs#6585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Adds more explicit documentation for the single optional parameter to process.hrtime to the process docs. PR-URL: #6585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Checklist
Affected core subsystem(s)
process
Description of change
Adds more explicit documentation for the single optional parameter to process.hrtime to the process docs. See #6571 for context.