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

Only check attributes of current tag for duplicates. #539

Merged
merged 2 commits into from
Jun 5, 2024
Merged

Only check attributes of current tag for duplicates. #539

merged 2 commits into from
Jun 5, 2024

Conversation

jdm
Copy link
Member

@jdm jdm commented Jun 5, 2024

Fixes #538. The current implementation never clears the present_attrs field so each unique pair of namespace and local names can only appear once in an XML document. The intent of the implementation is to exclude any attributes that resolve to a duplicate qualified name within a tag, so there is no need to store the hashset as a persistent field of the tree builder.

@jdm jdm force-pushed the xml-attrs branch 2 times, most recently from 26bbbf0 to 44a9110 Compare June 5, 2024 05:23
@jdm
Copy link
Member Author

jdm commented Jun 5, 2024

Depends on #540 to get rid of unrelated CI errors.

@demurgos
Copy link

demurgos commented Jun 5, 2024

I verified this fix locally, it fixes the issue of missing attributes.

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

This change makes sense to me. But would it be worth keeping the HashSet as a persistent property on the tree and just clearing it at the top of process_namespaces as an optimisation?

@jdm
Copy link
Member Author

jdm commented Jun 5, 2024

This change makes sense to me. But would it be worth keeping the HashSet as a persistent property on the tree and just clearing it at the top of process_namespaces as an optimisation?

I prefer the clarity of the current implementation. I'd be willing to change it if there's evidence that it's a performance gap that matters to a user, but I think that could be a followup.

@jdm jdm added this pull request to the merge queue Jun 5, 2024
Merged via the queue into main with commit 5de5ee9 Jun 5, 2024
6 checks passed
@mrobinson mrobinson deleted the xml-attrs branch June 5, 2024 21:37
@mrobinson
Copy link
Member

This caused 29 tests to start passing in Servo! 🎉

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.

xml5ever drops xml attributes with matching namespace and local names
4 participants