Wasmtime GitHub notifications bot (Jan 10 2025 at 10:46): Wasmtime GitHub notifications bot (Jan 10 2025 at 10:46):eagr requested fitzgen for a review on PR #9973.
Wasmtime GitHub notifications bot (Jan 10 2025 at 10:46):eagr requested wasmtime-compiler-reviewers for a review on PR #9973.
Wasmtime GitHub notifications bot (Jan 10 2025 at 10:46):eagr opened PR #9973 from eagr:isle-bnf-ext to bytecodealliance:main:
Address some issues in BNF
- \<ident\> overlapped with \<const-ident\>
(spec (signature ...))should be(spec (signature) ...)- spec expression def. missing
result
Wasmtime GitHub notifications bot (Jan 10 2025 at 12:54):github-actions[bot] commented on PR #9973:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jan 10 2025 at 22:48):cfallin submitted PR review:
Thanks -- two of the three grammar fixes here look good. Last one is a bit of a convention question (are identifiers with special meanings distinguished at the syntax level?) but I think I'd prefer to go the other way to minimize the grammar.
Wasmtime GitHub notifications bot (Jan 10 2025 at 22:48):cfallin created PR review comment:
This is considered an identifier and is not a separate keyword.
Wasmtime GitHub notifications bot (Jan 11 2025 at 05:12):eagr submitted PR review.
Wasmtime GitHub notifications bot (Jan 11 2025 at 05:12):eagr created PR review comment:
If it is an integral part of the verification and if produced value only bound to ident with the exact spelling "result", I think it deserves a place in the spec. Your call :))
Wasmtime GitHub notifications bot (Jan 11 2025 at 05:22):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 11 2025 at 05:22):cfallin created PR review comment:
It's currently special, but the intent has never really been to build up a set of keywords -- in the future we may find a better way. I don't see it quite the same as a privileged name like
thisin C++ orselfin Rust, for example.I think you may be placing too much weight on the BNF here, too: the purpose of BNF is usually not to give a parse tree with the full semantics of a language embedded at the parse level, but rather to describe the accepted language syntax accurately and give names to concepts that can then be given meaning by higher levels of the spec. That's more or less what is going on here.
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:07):eagr updated PR #9973.
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:08):eagr submitted PR review.
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:08):eagr created PR review comment:
I see. I certainly did and shouldn't. Thanks for pointing that out!
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:12):cfallin submitted PR review:
Thanks!
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:12):cfallin commented on PR #9973:
@eagr I see this is still marked as a draft; if you're ready for me to merge it, I'm happy to do so, you'll just need to mark it as ready.
Wasmtime GitHub notifications bot (Jan 11 2025 at 06:24):Sure! I'm writing a tree-sitter parser for ISLE and picking up inconsistencies from the BNF. Thought it might be nice to correct those along the way. I'll flag it as ready when I'm done.
Wasmtime GitHub notifications bot (Jan 12 2025 at 07:17):eagr updated PR #9973.
Wasmtime GitHub notifications bot (Jan 12 2025 at 17:12):Nitpick
- \<int\> isn't accurate enough, e.g., "0x_" is invalid
- "switch" probably shouldn't be \<spec_op\> if \<spec_pair\> isn't a \<spec_expr\>
Do you want to address these too?
Wasmtime GitHub notifications bot (Jan 12 2025 at 17:13):eagr updated PR #9973.
Wasmtime GitHub notifications bot (Jan 12 2025 at 17:14):eagr has marked PR #9973 as ready for review.
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:26):cfallin submitted PR review:
Let's get this merged as-is and then any spec-related grammar fixes I think I might delegate to @avanhatt or @mmcloughlin to review -- their project is really authoritative on how that sublanguage is designed. Thanks for these fixes!
Wasmtime GitHub notifications bot (Jan 13 2025 at 20:47):cfallin merged PR #9973.
Last updated: Jan 24 2025 at 00:11 UTC