Uh oh!
There was an error while loading. Please reload this page.
timers: give Timeouts a constructor name - #5793
Conversation
cjihrig
commented
Mar 18, 2016
LGTM |
jasnell
commented
Mar 19, 2016
should be safe for v4, no? |
There was a problem hiding this comment.
Any reason not to make it a class Timeout?
classTimeout{constructor(after){this._called=false;this._idleTimeout=after;this._idlePrev=this;this._idleNext=this;this._idleStart=null;this._onTimeout=null;this._repeat=null;}unref(){if(this._handle){this._handle.unref();}elseif(typeofthis._onTimeout==='function'){varnow=TimerWrap.now();if(!this._idleStart)this._idleStart=now;vardelay=this._idleStart+this._idleTimeout-now;if(delay<0)delay=0;// Prevent running cb again when unref() is called during the same cbif(this._called&&!this._repeat){exports.unenroll(this);return;}varhandle=reuse(this);this._handle=handle||newTimerWrap();this._handle.owner=this;this._handle[kOnTimeout]=functionunrefdHandle(){this.owner._onTimeout();if(!this.owner._repeat)this.owner.close();};this._handle.start(delay,0);this._handle.domain=this.domain;this._handle.unref();}returnthis;}ref(){if(this._handle)this._handle.ref();returnthis;}close(){this._onTimeout=null;if(this._handle){this._handle[kOnTimeout]=null;this._handle.close();}else{exports.unenroll(this);}returnthis;}}There was a problem hiding this comment.
No gain, more code change.
There was a problem hiding this comment.
Well, it modernizes things a little and makes them more explicit. If we want to minimize change why not keep it const Timeout and do const Timeout = function Timeout - by naming the function expression you get the same trace.
There was a problem hiding this comment.
Why even assign it though? It's unnecessary.
Typically we have avoided modernization unless it meant performance increases or was part of some refactor that touched the code a lot anyways.
There was a problem hiding this comment.
Fair enough - I'm sorry if all these questions annoy you :)
ChALkeR
commented
Mar 19, 2016
LGTM |
Refs: nodejs#5792 PR-URL: nodejs#5793 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Fixes: nodejs#5823 Refs: nodejs#5793 PR-URL: nodejs#5825 Reviewed-By: Myles Borins <myles.borins@gmail.com>
Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) #5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) #5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) #4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) #5800 PR-URL: #5831
Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) #5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) #5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) #4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) #5800 PR-URL: #5831
Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) #5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) #5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) #4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) #5800 PR-URL: #5831
Notable changes: * buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt Loring) nodejs#5605 - This effects write{Float|Double} when the noAssert option is not used. * timers: - Returned timeout objects now have a Timeout constructor name (Jeremiah Senkpiel) nodejs#5793 - Performance of Immediate processing is now ~20-40% faster (Brian White) nodejs#4169 * vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz Sheikh) nodejs#5800 PR-URL: nodejs#5831
MylesBorins
commented
Mar 30, 2016
@Fishrock123 can you confirm that this is safe and ready for v4? |
MylesBorins
commented
Mar 30, 2016
if this does land we should apply the lint fixes found in #5825 |
Fishrock123
commented
Apr 4, 2016
@thealphanerd should be safe. It's a sorta-feature, but will only show up in logs I think. It's not even exported, so you can't do instanceof checks with it. |
MylesBorins
commented
Aug 30, 2016
@Fishrock123 I'm going to defer to your judgement here. Please feel free to send a backport PR or change the label |
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
Affected core subsystem(s)
timers
Description of change
Refs: #5792
No functional difference, this is for clarity only.