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

Fix a few minor issues in local validation rules #108

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

takikawa
Copy link
Contributor

Here are the current rendered local rules:

image

From this PR:

image

The main obvious fix is for the LTYPE which is misformatted. Two other fixes:

  • I suspect the ok in the conclusion of the validation rule isn't needed, as there's an output.
  • In the function { type x, locals t*, body expr }, the t* are locals with shape {type t'} so I think there needs to be some matching on that somewhere in the rule? Here I put it in the conclusion but it could go in the premise too.

BTW: the spacing on the set/unset seem a bit off too. I think it's due to the \mathrel used for those keywords but wasn't sure what the preferred change was there.

Add missing backslash on \LTYPE

Local validation doesn't need `ok`

Locals in a function have structure so need {type t} to
match the valtype
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks!

Can you change the SET/UNSET macros in util/macros.def to \K{set} and \K{unset}, without the mathrel?

document/core/valid/modules.rst Outdated Show resolved Hide resolved
@@ -48,11 +48,11 @@ Functions :math:`\func` are classified by :ref:`type indices <syntax-typeidx>` r
\frac{
C.\CTYPES[x] = [t_1^\ast] \toF [t_2^\ast]
\qquad
(C \vdashlocal t : \init~t)^\ast
(C \vdashlocal \{\LTYPE~t\} : \init~t)^\ast
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I forgot to add the {type t} here too, to match the validation rules below.

@rossberg rossberg merged commit 959a618 into WebAssembly:main Oct 31, 2023
4 checks passed
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.

2 participants