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

Reorganize the "Canonical Built-ins" section of Explainer.md. #413

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

Conversation

sunfishcode
Copy link
Member

This moves the signatures for the canonical builtins out of prose paragraphs and into tables, which highlights them and simplifies the prose.

This also adds "conceptual signatures". The canonical ABI signatures are useful when one is writing compilers or bindings generators or other tools, while the conceptual signatures are useful for people seeking to understand what these builtins do at a conceptual level, before thinking about how all the various values are lowered into i32s.

And, this PR introduces greater separation between the logical builtins and the way they appear in the canonical ABI, with the conceptual signatures, and with prose that puts canonical ABI details in their own paragraphs. In theory, this should make it easier to reuse the logical builtins when adding new ABIs.

This is a follow-up to #407; I've re-added the canon ebpf for now, removed the "Binary encoding immediates" table rows, and tidied up the signatures to avoid using ....

This moves the signatures for the canonical builtins out of prose paragraphs
and into tables, which highlights them and simplifies the prose.

This also adds "conceptual signatures". The canonical ABI signatures are
useful when one is writing compilers or bindings generators or other tools,
while the conceptual signatures are useful for people seeking to
understand what these builtins do at a conceptual level, before thinking
about how all the various values are lowered into `i32`s.

And, the conceptual signatures in theory are independent of the ABI, so
they could be reused when new ABIs are added.

This is a follow-up to WebAssembly#407; I've re-added the
`canon` ebpf for now, removed the "Binary encoding immediates" table rows,
and tidied up the signatures to avoid using `...`.
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks so much for doing this! I just have a few nits (and pre-existing doc fixes), and also a few syntax ideas that I was curious to see spelled out before merging.

design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Good changes! A few more comments:

design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved

| Synopsis | |
| -------------------------- | ----------------------------------------------------------- |
| Approximate WIT signature | `func<T>(stream: readable-stream<T>, buffer: readable-buffer<T>) -> read-status` |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Approximate WIT signature | `func<T>(stream: readable-stream<T>, buffer: readable-buffer<T>) -> read-status` |
| Approximate WIT signature | `func<T>(stream: borrow<readable-stream<T>>, buffer: writable-buffer<T>) -> read-status` |

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is the readable-stream<T> explicitly borrow, when writable-buffer<T> is not?

design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
@lann
Copy link
Contributor

lann commented Nov 19, 2024

Thanks for this! I had been reading through the async content and compiling a list of questions before seeing this PR but I think they're mostly resolved here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants