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

[Parser] Allow any number of foldedinsts in foldedinsts #5965

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Sep 20, 2023

Somewhat counterintuitively, the text syntax for a folded if allows any number
of folded instructions in the condition position, not just one. Update the
corresponding foldedinsts parsing function to parse arbitrary sequences of
folded instructions and add a test.

@tlively
Copy link
Member Author

tlively commented Sep 20, 2023

@tlively
Copy link
Member Author

tlively commented Sep 20, 2023

Diff is smaller without whitespace.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #5965 (f10684c) into main (2c09a60) will decrease coverage by 0.01%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main    #5965      +/-   ##
==========================================
- Coverage   43.00%   43.00%   -0.01%     
==========================================
  Files         492      492              
  Lines       74875    74879       +4     
  Branches    11656    11658       +2     
==========================================
+ Hits        32200    32201       +1     
- Misses      39450    39452       +2     
- Partials     3225     3226       +1     
Files Changed Coverage Δ
src/parser/input.h 66.66% <ø> (ø)
src/parser/parsers.h 51.64% <51.61%> (-0.01%) ⬇️
src/parser/input-impl.h 62.60% <66.66%> (+0.10%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Yikes, the wasm text format is... tricky.

I understand where this particular complexity comes from, but I had not realized it was possible until now.

return false;
}
bool ret = takeKeyword(expected);
lexer = original;
Copy link
Member

Choose a reason for hiding this comment

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

How efficient is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very. It's essentially just resetting an index into the source buffer to its previous value.

@tlively tlively mentioned this pull request Sep 20, 2023
@tlively
Copy link
Member Author

tlively commented Sep 21, 2023

See also WebAssembly/spec#1682

@tlively tlively force-pushed the parser-fix-foldedinsts branch from fdf17e2 to be155c7 Compare September 21, 2023 01:53
@tlively tlively force-pushed the parser-simplify-instrs branch from a679fee to fe1a81a Compare September 21, 2023 19:56
@tlively tlively force-pushed the parser-fix-foldedinsts branch from be155c7 to 65ee4c5 Compare September 21, 2023 19:57
Base automatically changed from parser-simplify-instrs to main September 21, 2023 20:46
Somewhat counterintuitively, the text syntax for a folded `if` allows any number
of folded instructions in the condition position, not just one. Update the
corresponding `foldedinsts` parsing function to parse arbitrary sequences of
folded instructions and add a test.
@tlively tlively force-pushed the parser-fix-foldedinsts branch from 65ee4c5 to f10684c Compare September 21, 2023 20:49
@tlively
Copy link
Member Author

tlively commented Sep 21, 2023

I found a much simpler way to implement this. PTAL!

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Nice!

@tlively tlively merged commit 38197b5 into main Sep 21, 2023
@tlively tlively deleted the parser-fix-foldedinsts branch September 21, 2023 22:33
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
…y#5965)

Somewhat counterintuitively, the text syntax for a folded `if` allows any number
of folded instructions in the condition position, not just one. Update the
corresponding `foldedinsts` parsing function to parse arbitrary sequences of
folded instructions and add a test.
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