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 mutability from parser/tokenizer APIs. #548

Merged
merged 8 commits into from
Aug 7, 2024
Merged

Remove mutability from parser/tokenizer APIs. #548

merged 8 commits into from
Aug 7, 2024

Conversation

jdm
Copy link
Member

@jdm jdm commented Aug 1, 2024

This is incompatible with a re-entrant parsing algorithm which is required by the web platform's document.write API.

@jdm
Copy link
Member Author

jdm commented Aug 1, 2024

FYI @Taym95

@Taym95
Copy link
Contributor

Taym95 commented Aug 1, 2024

FYI @Taym95

Thanks, Should I close the other PR? lets me knew is you need help here in clean up.

Some(x) => x,
{
let Some(x) = $opt else { $else_block };
x
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is necessary to minimize the scope of refcell borrows that are passed as arguments to the macro.

@jdm
Copy link
Member Author

jdm commented Aug 2, 2024

FYI @Taym95

Thanks, Should I close the other PR? lets me knew is you need help here in clean up.

We can still merge the other PR, but we should remove all uses of &mut BufferQueue from the codebase in it (and uses like &mut input).

@jdm jdm marked this pull request as ready for review August 7, 2024 03:41
@jdm jdm force-pushed the no-mut-interface branch 2 times, most recently from 5858c9b to 6165801 Compare August 7, 2024 03:48
@jdm
Copy link
Member Author

jdm commented Aug 7, 2024

Ok, the code's cleaned up and commits are split into:

  1. mechanical changes - lots of removing &mut self from methods and adding Cell/RefCell
  2. new APIs needed by Support HTML parser reentrancy servo#32820
  3. functional changes - the TreeSink API for obtaining an element's name relies on borrowed data in ways that do not play nicely with naive usage of RefCell to store the element name data (ExpandedName docs)
  4. (and remaining commits) getting CI to pass

@jdm
Copy link
Member Author

jdm commented Aug 7, 2024

@Taym95 Would you like to review this?

Copy link
Contributor

@Taym95 Taym95 left a comment

Choose a reason for hiding this comment

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

I've reviewed all the changes, and everything looks good to me!

@jdm jdm added this pull request to the merge queue Aug 7, 2024
Merged via the queue into main with commit 801be63 Aug 7, 2024
6 checks passed
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.

3 participants