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

Remove $idx #122

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Remove $idx #122

merged 3 commits into from
Sep 6, 2024

Conversation

ShinWonho
Copy link

@ShinWonho ShinWonho commented Sep 3, 2024

Hello,
There had been a spec bug in ref.null, so ref.null algorithm was hardcoded in AL interpreter. Now the bug is fixed, I'm trying to remove the hardcoding. However, there is a problem.

rule Step_read/ref.null-idx:
  z; (REF.NULL $idx(x))  ~>  (REF.NULL $type(z, x))
Step_read/ref.null $idx(x)
1. Let z be the current state.
2. Push the value (REF.NULL $type(z, x)) to the stack.

This is current reduction rule and prose of ref.null. There is a function call $idx(x) in the LHS. The problem is, we cannot pattern match on a function call.

One possible solution is to use inverse function, and another is not to use function call in LHS.

  1. To use inverse function
Step_read/ref.null typevar
1. Let z be the current state.
2. Let x be $idx^-1(typevar).
3. Push the value (REF.NULL $type(z, x)) to the stack.
  1. Not to use function call in lhs
rule Step_read/ref.null-idx:
  z; (REF.NULL (_IDX x))  ~>  (REF.NULL $type(z, x))
Step_read/ref.null (_IDX x)
1. Let z be the current state.
2. Push the value (REF.NULL $type(z, x)) to the stack.

I think the prose of the second version looks better, but this version means that function call is not allowed in LHS of reduction rule. Do you think the constraint is reasonable, @rossberg?

@ShinWonho ShinWonho requested a review from rossberg September 3, 2024 07:55
@rossberg
Copy link
Collaborator

rossberg commented Sep 4, 2024

Yeah, this function is a hack, the only reason is exists is that it will otherwise render redundant parentheses around x, i.e., you'll see "(ref.null (x))" instead of "(ref.null x)".

Maybe a better way to solve this problem is to either special-case rendering of parens around hidden constructors, or introduce some form of hidden parentheses.

As a temporary solution to unblock you, I'm okay with removing this call site for now. But please add a TODO assigned to me to fix rendering. :)

@ShinWonho
Copy link
Author

ShinWonho commented Sep 4, 2024

Screenshot 2024-09-04 at 7 29 54 PM

I just want a double-check. I built the document, but it seems that there are no redundant parentheses. Does the problem still exist?

@rossberg
Copy link
Collaborator

rossberg commented Sep 4, 2024

Oh. Looking back at the code, it turns I already implemented that special case in the renderer. :) So the $idx function is completely unnecessary now. Would you mind removing it everywhere?

@ShinWonho
Copy link
Author

ShinWonho commented Sep 4, 2024

No! I'll remove all $idx :)

@ShinWonho ShinWonho changed the title Remove function call in LHS Remove $idx Sep 4, 2024
@ShinWonho ShinWonho merged commit a2240b2 into main Sep 6, 2024
8 checks passed
@ShinWonho ShinWonho deleted the ref.null branch September 9, 2024 07:16
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