-
Notifications
You must be signed in to change notification settings - Fork 19
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 by Bors] - Fix for correctly parsing closing tags #38
Conversation
The previous implementation of reading closing tags did not ignore voided tags. causing the parent tag to be closed prematurely.
Okay, I did some testing/benchmarking to see if it regresses perf and I got some very strange results. After updating to latest nightly, the benchmark performed consistently significantly worse for some reason (~12-18% slower) rustc 1.60.0-nightly (ad46af247 2022-01-14):
This PR:
rustc 1.62.0-nightly (878c7833f 2022-04-16):
This PR:
Though this isn't really related to this PR and more an issue affecting the library as a whole. |
…l spec. According to the W3C html spec, having whitespace after/before the '/' for the closing tag is not valid. Since we assume we are parsing well-formed html docs we skip any seeking to the tag name.
I guess the only way to test this would be to compare the version in this pull request with the version on crates.io. Then we can narrow down if this pull request is the culprit. I would not expect such a huge hit as well. |
Nah, I think this PR is fine. I think my previous comment was a bit unclear and it's not related to this PR at all. I'd understand if this PR makes it a little slower because it has to do more work, but what confused me was that the same tl version that is on crates.io consistently runs around 15% slower on newer nightly builds compared to a nightly build from january. I'll try to track it down some day to see in which nightly build this started happening. Probably deserves its own tracking issue too. I actually managed to get the perf of this fix "roughly" on par with the version on crates.io by changing one thing (will add a review for this), which surprised me, considering it checks the name of every tag in the average case, but I guess you really can't predict perf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for reporting the bug and the fix!
bors r+
Fixes #37 This pull request fixes the issue where void closing tags would not be ignored. Causing the closing tag to close the parent instead of the voided tag. To implement this, I had to also fix the ``tag_raw_abrupt_stop`` test. Because this would not treat the ``eof`` as a valid end of a identifier. Causing the identifier parsing of the closing tag to fail.
No problem! helping fixing bugs let's me learn as well. |
Pull request successfully merged into master. Build succeeded: |
published a new release (0.7.5) to crates.io with this fix. |
Fixes #37
This pull request fixes the issue where void closing tags would not be ignored. Causing the closing tag to close the parent instead of the voided tag.
To implement this, I had to also fix the
tag_raw_abrupt_stop
test. Because this would not treat theeof
as a valid end of a identifier. Causing the identifier parsing of the closing tag to fail.