Skip to content

Fix XML parsing in WASI environment #4683

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

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

kellyo-cricut
Copy link
Contributor

Final chunk of XML document was being skipped when parsing when in a WASI environment

Final chunk of XML document was being skipped when parsing when in a WASI environment
@kateinoigakukun
Copy link
Member

@swift-ci please test

@kateinoigakukun
Copy link
Member

CC: @MaxDesiatov

@kateinoigakukun
Copy link
Member

@swift-ci please test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Would be great to cover this with a test, but I'm not sure we have testing infrastructure for WASI in this repository. Otherwise seems legit.

@MaxDesiatov
Copy link
Contributor

@swift-ci test macOS

@kellyo-cricut
Copy link
Contributor Author

I can't quite tell from the output of the macOS action- Are there legitimate test failures I need to address?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Jan 10, 2023

The test failure seems unrelated to me, but I requested at least one more review just in case.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test macOS platform

1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci please test macOS platform

if chunkStart >= data.count || chunkEnd >= data.count {
break
}
while result && chunkStart != chunkEnd {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks safe based on reading the code below. One reason why I personally stick with < checks in places like this instead of != is that if a later change or some missed path advances beyond the end, we fail safely instead of entering an infinite loop.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov MaxDesiatov merged commit 7504fdf into swiftlang:main Feb 2, 2023
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.

4 participants