Closed
Bug 1470490
Opened 6 years ago
Closed 6 years ago
Rename Atomics.wake to Atomics.notify per ES standard change
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: rwaldron, Assigned: lth)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
16.41 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
At the May 2018 TC39 meeting, consensus was reached to rename Atomics.wake to Atomics.notify (while the API is temporarily disabled). The spec change can be found here: https://github.com/tc39/ecma262/pull/1220
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Updated•6 years ago
|
Summary: Rename (or alias) Atomics.wake => Atomics.notify → Rename Atomics.wake to Atomics.notify per ES standard change
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•6 years ago
|
||
I was going to flag this as good-first-bug, but as it also affects asm.js I'm not actually sure if that's true...
Assignee | ||
Comment 2•6 years ago
|
||
Let's nuke asm.js's atomics first, then we don't have to worry so much. Also it may behoove us to keep an alias for Atomics.wake for a while? Not sure.
Depends on: 1423577
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•6 years ago
|
||
asm.js atomics are gone; only straight JS needs to be updated. (Wasm atomics are unaffected by JS naming changes, plus Wasm is a binary format.)
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 4•6 years ago
|
||
Before we re-enable SAB we should be sure to correct the naming.
Blocks: resab
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9002194 -
Flags: review?(andrebargull)
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Comment 6•6 years ago
|
||
Comment on attachment 9002194 [details] [diff] [review] bug1470490-notify-is-the-new-wake.patch Review of attachment 9002194 [details] [diff] [review]: ----------------------------------------------------------------- > Do we never run shell tests marked "slow"? No, I don't think so. r+ with the Atomics changes from <https://hg.mozilla.org/mozilla-central/rev/317099325f4c> reverted. (You may want to wait for bug 1484728, because it fixes a typo in a test262 Atomics test.) ::: js/src/builtin/AtomicsObject.cpp @@ +982,5 @@ > JS_INLINABLE_FN("xor", atomics_xor, 3,0, AtomicsXor), > JS_INLINABLE_FN("isLockFree", atomics_isLockFree, 1,0, AtomicsIsLockFree), > JS_FN("wait", atomics_wait, 4,0), > + JS_FN("notify", atomics_notify, 3,0), > + JS_FN("wake", atomics_notify, 3,0), // Legacy name Creating two separate functions is easier, but I wonder if it makes sense to have only one function installed as two separate properties, similar to how |String.prototype.trimLeft| is the same function as |String.prototype.trimStart| [1] or how |Date.prototype.toGMTString| is the same function as |Date.prototype.toUTCString| [2]. [1] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/builtin/String.cpp#3649-3657 [2] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/jsdate.cpp#3389-3398 ::: js/src/builtin/AtomicsObject.h @@ +78,4 @@ > // times allowed; specify mozilla::Nothing() for an indefinite > // wait. > // > + // notify() will not wake up spuriously. It will return true and Shouldn't this still be "wait()"? Pre-existing: |return true| no longer correct since bug 1377576. @@ +82,1 @@ > // set *result to a return code appropriate for Pre-existing: |*result| no longer exists. @@ +82,2 @@ > // set *result to a return code appropriate for > // Atomics.wait() on success, and return false on error. Ditto: |return false|.
Attachment #9002194 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to André Bargull [:anba] from comment #6) > Comment on attachment 9002194 [details] [diff] [review] > bug1470490-notify-is-the-new-wake.patch > > Review of attachment 9002194 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/builtin/AtomicsObject.cpp > @@ +982,5 @@ > > JS_INLINABLE_FN("xor", atomics_xor, 3,0, AtomicsXor), > > JS_INLINABLE_FN("isLockFree", atomics_isLockFree, 1,0, AtomicsIsLockFree), > > JS_FN("wait", atomics_wait, 4,0), > > + JS_FN("notify", atomics_notify, 3,0), > > + JS_FN("wake", atomics_notify, 3,0), // Legacy name > > Creating two separate functions is easier, but I wonder if it makes sense to > have only one function installed as two separate properties, similar to how > |String.prototype.trimLeft| is the same function as > |String.prototype.trimStart| [1] or how |Date.prototype.toGMTString| is the > same function as |Date.prototype.toUTCString| [2]. In this case I'll leave it the way it is since there will be a followup bug to get rid of the wake() alias once we're satisfied that we're not breaking anything in our test suite. (Nice observation re pre-existing bogus prose.)
Assignee | ||
Comment 8•6 years ago
|
||
Includes the fix for jstests.list and fixes the comment nits. Leaves wake and notify as two distinct functions. Ready to land but we need to wait for the blockers.
Attachment #9002194 -
Attachment is obsolete: true
Attachment #9002760 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jwalden+bmo)
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d4bf8a92b3 Introduce Atomics.notify, make Atomics.wake an alias. r=anba
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d9d4bf8a92b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 11•6 years ago
|
||
Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/notify
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•