Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

gibson042
Copy link
Contributor

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.

@jmdyck
Copy link
Collaborator

jmdyck commented Aug 16, 2024

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".

@gibson042 gibson042 force-pushed the 2024-08-cleanup-memory-model branch from 3097e97 to 8e2797a Compare August 16, 2024 18:22
@gibson042
Copy link
Contributor Author

@jmdyck Thanks, updated.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@syg syg added the editor call to be discussed in the next editor call label Aug 20, 2024
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % remaining comment

@gibson042 gibson042 force-pushed the 2024-08-cleanup-memory-model branch from d03bcb1 to a3b78dd Compare August 22, 2024 16:05
@gibson042
Copy link
Contributor Author

Thanks for the thorough review, @syg. I've incorporated the remaining suggests and rebased.

@syg syg removed the editor call to be discussed in the next editor call label Aug 28, 2024
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_).
Copy link
Member

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.

Suggested change
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_).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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_).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

@gibson042 gibson042 Sep 3, 2024

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.

Copy link
Member

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.

@gibson042
Copy link
Contributor Author

@michaelficarra Thanks for the suggestions. Updated.

Copy link
Contributor

@bakkot bakkot left a 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.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Sep 3, 2024
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants