-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Editorial: Miscellaneous improvements to memory model/(Shared)ArrayBuffer/TypedArray #3396
base: main
Are you sure you want to change the base?
Conversation
There are three occurrences of "For each event" that should probably be changed to "For each Memory event". And similarly one occurrence of "there exists an event". |
3097e97
to
8e2797a
Compare
@jmdyck Thanks, updated. |
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.
lgtm % remaining comment
…ord [[EventList]] contents (Shared Data Block events, Synchronize events, and a host-specific events)
…Data Block event comparison
…linked "{reads,writes}"
d03bcb1
to
a3b78dd
Compare
Thanks for the thorough review, @syg. I've incorporated the remaining suggests and rebased. |
1. For each event _E_ of EventSet(_execution_), do | ||
1. If _E_ is not in SharedDataBlockEventSet(_execution_), add _E_ to _events_. | ||
1. Return _events_. | ||
1. Return the subtraction of SharedDataBlockEventSet(_execution_) from EventSet(_execution_). |
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'd prefer not to rely on this novel phrasing. It's just as easy to be explicit about it.
1. Return the subtraction of SharedDataBlockEventSet(_execution_) from EventSet(_execution_). | |
1. Return a new Set that contains all elements of EventSet(_execution_) that are not in SharedDataBlockEventSet(_execution_). |
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.
1. Return the subtraction of SharedDataBlockEventSet(_execution_) from EventSet(_execution_). | |
1. Return a new Set containing all elements of EventSet(_execution_) that are not in SharedDataBlockEventSet(_execution_). |
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.
WFM
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.
What do you dislike about subtraction @michaelficarra?
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 don't dislike it at all, but if we did use it, I'd at least want to say somewhere in the Set section that this is a thing we can do (and possibly explicitly define it). Instead though, it's basically just as easy to write it out.
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.
Oh, it already does say that: The Set and Relation Specification Types (emphasis mine)
Sets may be unioned, intersected, or subtracted from each other.
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.
Oh! I didn't know that was there. In that case, yeah, using subtraction is fine.
@michaelficarra Thanks for the suggestions. Updated. |
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.
LGTM except that I really don't like the different capitalization on "Memory event" vs "memory range". These are using "memory" in the same sense, and should match.
A sequence of mostly-independent commits (separable upon request) that fixes some typos, improves internal consistency, and takes better advantage of auto-linking to explain the memory model and the buffer/view operations that build upon it.