-
Notifications
You must be signed in to change notification settings - Fork 514
fix: INTERPRETING.md broadcast and receiveBroadcast function documentation is out of date #4693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -85,42 +85,52 @@ properties of the global scope prior to test execution. | |||||||||||||||||||||||||
| running. The agent has no representation. The agent script will be | ||||||||||||||||||||||||||
| run in an environment that has an object `$262` with a property `agent` | ||||||||||||||||||||||||||
| with the following properties: | ||||||||||||||||||||||||||
| - **`receiveBroadcast`** - a function that takes a function and | ||||||||||||||||||||||||||
| calls the function when it has received a broadcast from the parent, | ||||||||||||||||||||||||||
| passing it the broadcast as two arguments, a SharedArrayBuffer and | ||||||||||||||||||||||||||
| an Int32 or BigInt. This function may return before a broadcast is received | ||||||||||||||||||||||||||
| (eg to return to an event loop to await a message) and no code should | ||||||||||||||||||||||||||
| follow the call to this function. | ||||||||||||||||||||||||||
| - **`receiveBroadcast`** - a function that takes a callback and | ||||||||||||||||||||||||||
| calls the callback when it has received a broadcast from the parent, | ||||||||||||||||||||||||||
| passing it the broadcast as a single SharedArrayBuffer. This function | ||||||||||||||||||||||||||
| may return before a broadcast is received (eg. to return to an event | ||||||||||||||||||||||||||
| loop to await a message) and no code should follow the call to this | ||||||||||||||||||||||||||
| function. | ||||||||||||||||||||||||||
|
Comment on lines
+88
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| - **`report`** - a function that accepts a single "message" argument, | ||||||||||||||||||||||||||
| which is converted to a string\* and placed in a transmit queue whence the parent will retrieve it. Messages should be short. (\* Note that string conversion has been implicit since the introduction of this host API, but is now explicit.) | ||||||||||||||||||||||||||
| which is converted to a string and placed in a transmit queue whence | ||||||||||||||||||||||||||
| the parent will retrieve it. Messages should be short. (Note that | ||||||||||||||||||||||||||
| string conversion has been implicit since the introduction of this host | ||||||||||||||||||||||||||
| API, but is now explicit.) | ||||||||||||||||||||||||||
| - **`sleep`** - a function that takes a millisecond argument and | ||||||||||||||||||||||||||
| sleeps the agent for approximately that duration. | ||||||||||||||||||||||||||
| - **`leaving`** - a function that signals that the agent is done and | ||||||||||||||||||||||||||
| may be terminated (if possible). | ||||||||||||||||||||||||||
| - **`monotonicNow`** - a function that returns a value that conforms to [`DOMHighResTimeStamp`][] and is produced in such a way that its semantics conform to **[Monotonic Clock][]**. | ||||||||||||||||||||||||||
| - **`broadcast`** - a function that takes a SharedArrayBuffer and an | ||||||||||||||||||||||||||
| Int32 or BigInt and broadcasts the two values to all concurrent | ||||||||||||||||||||||||||
| agents. The function blocks until all agents have retrieved the | ||||||||||||||||||||||||||
| message. Note, this assumes that all agents that were started are | ||||||||||||||||||||||||||
| still running. | ||||||||||||||||||||||||||
| - **`getReport`** - a function that reads an incoming string from any agent, | ||||||||||||||||||||||||||
| and returns it if it exists, or returns `null` otherwise. | ||||||||||||||||||||||||||
| - **`monotonicNow`** - a function that returns a value that conforms to | ||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`] and is produced in such a way that its | ||||||||||||||||||||||||||
| semantics conform to **[Monotonic Clock]**. | ||||||||||||||||||||||||||
| - **`broadcast`** - a function that takes a SharedArrayBuffer and | ||||||||||||||||||||||||||
| broadcasts it to all concurrent agents. The function blocks until all | ||||||||||||||||||||||||||
| agents have retrieved the message. Note, this assumes that all agents | ||||||||||||||||||||||||||
| that were started are still running. | ||||||||||||||||||||||||||
|
Comment on lines
+106
to
+109
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise here, dropping the
Comment on lines
+106
to
+109
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| - **`getReport`** - a function that reads an incoming string from any | ||||||||||||||||||||||||||
| agent, and returns it if it exists, or returns `null` otherwise. | ||||||||||||||||||||||||||
|
Comment on lines
+110
to
+111
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| - **`sleep`** - a function that takes a millisecond argument and | ||||||||||||||||||||||||||
| sleeps the execution for approximately that duration. | ||||||||||||||||||||||||||
| - **`monotonicNow`** - a function that returns a value that conforms to [`DOMHighResTimeStamp`][] and is produced in such a way that its semantics conform to **[Monotonic Clock][]**. | ||||||||||||||||||||||||||
| - **`monotonicNow`** - a function that returns a value that conforms to | ||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`] and is produced in such a way that its | ||||||||||||||||||||||||||
| semantics conform to **[Monotonic Clock]**. | ||||||||||||||||||||||||||
|
Comment on lines
+114
to
+116
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| In addition, consumers may choose to override any of [the functions defined by test harness files](https://github.com/tc39/test262/blob/HEAD/CONTRIBUTING.md#test-environment) as they see fit. See [the documentation on handling errors and negative test cases](https://github.com/tc39/test262/blob/HEAD/CONTRIBUTING.md#handling-errors-and-negative-test-cases) for a useful example of this. | ||||||||||||||||||||||||||
| In addition, consumers may choose to override any of [the functions defined by | ||||||||||||||||||||||||||
| test harness files](https://github.com/tc39/test262/blob/HEAD/CONTRIBUTING.md#test-environment) | ||||||||||||||||||||||||||
| as they see fit. See [the documentation on handling errors and negative test | ||||||||||||||||||||||||||
| cases](https://github.com/tc39/test262/blob/HEAD/CONTRIBUTING.md#handling-errors-and-negative-test-cases) | ||||||||||||||||||||||||||
| for a useful example of this. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #### Normative references | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`][], **[Monotonic Clock][]**<br> | ||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`], **[Monotonic Clock]**<br> | ||||||||||||||||||||||||||
| Ilya Grigorik, James Simonsen, Jatinder Mann.<br> | ||||||||||||||||||||||||||
| [High Resolution Time Level 2](https://www.w3.org/TR/hr-time-2/) March 2018. W3C. URL: [https://www.w3.org/TR/hr-time-2/](https://www.w3.org/TR/hr-time-2/) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`]: https://www.w3.org/TR/hr-time-2/#sec-domhighrestimestamp "**DOMHighResTimeStamp**" | ||||||||||||||||||||||||||
| [Monotonic Clock]: https://www.w3.org/TR/hr-time-2/#sec-monotonic-clock "**Monotonic Clock**" | ||||||||||||||||||||||||||
| [`DOMHighResTimeStamp`]: https://www.w3.org/TR/hr-time-2/#sec-domhighrestimestamp "**DOMHighResTimeStamp**" | ||||||||||||||||||||||||||
| [Monotonic Clock]: https://www.w3.org/TR/hr-time-2/#sec-monotonic-clock "**Monotonic Clock**" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Strict Mode | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true AFAICT that no test currently in this suite defines a
receiveBroadcastcallback that uses its second argument, but that argument is provided. Here are some example runtime implementations:And per the top of this file, relaxing the requirement to provide that second argument would need to come with a package.json version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was rather watching this from the point of view of how the
.broadcastfunction is called: it's only called withinsafeBroadcastandsafeBroadcastAsyncand never with a second parameter. So either the second parameter doesn't exist, or it has a third "None" value.I wrote my implementation of the function based on these descriptions and assumed that this must be talking about passing a TypedArray to the receiver (as it has originally been AFAICT), and made the function check that the argument really is a (shared) TypedArray: whoops nope, an error is thrown. Okay, so it expects a SharedArrayBuffer and a second argument which I guess is a Number? Nope, still an error. What am I actually getting as a parameter? Oh,
undefinedevery time.Maybe this documentation isn't out of date per se, but it definitely is wrong. There is no second parameter currently.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be clear on vocabulary—"parameter" is part of the signature of a function, while "argument" is part of an invocation. As mentioned above, no test in this repository currently specifies a
receiveBroadcastfunction that uses a second argument (or indeed defines a second parameter at all), and no invocation ofbroadcastcurrently supplies a second argument—which, per normal ECMAScript semantics about absent arguments having valueundefined, therefore means that all invocations ofreceiveBroadcastthat are in conformance with INTERPRETING.md must receive either a single argument or a pair of arguments in which the second isundefined.I agree that the text could be more clear. What I'm inferring based upon both it and actual implementations is that the second parameter is optional—
broadcastis a function that takes a SharedArrayBuffer and an optional Int32 or BigInt and broadcasts the two values (the SharedArrayBuffer and a number or bigint orundefined) to all concurrent agents.There most certainly is a second parameter, and any implementation that does not pass along a bigint or int32-representing number provided in an invocation of
broadcastto the corresponding invocation(s) ofreceiveBroadcastwould be out of conformance with the documented requirements.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I apologise most dreadfully. This is unacceptable and truly heinous of me.
I actually only only now realised to me that the second parameter is even supposed to be an integer. I had assumed the phrasing "BigInt or Int32" was listing two enumeration variants, instead of listing out two acceptable types for the second argument.
So; should I close the PR? Should I bump the package.json version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should repurpose this PR into a clarification of the existing requirements that does not change them.