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

RFC: Drop tokenizer::Token intermediate data structure and XmlEvent trait #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adamreichold
Copy link
Collaborator

As discussed previously here, this simplifies the code base, especially the interaction between tokenizer and parser. However, it also appears to imply that we have to drop the tokenizer test which intimately rely on the intermediate token representation.

Further code clean-ups are possible, e.g. standardizing on methods on Context instead of free functions, but this change aims for a minimal change while we discuss this.

This simplifies the code base, especially the interaction between tokenizer
and parser. However, it also appears to imply that we have to drop the tokenizer
test which intimately rely on the intermediate token representation.

Further code clean-ups are possible, e.g. standardizing on methods on Context
instead of free functions, but this change aims for a minimal change while
we discuss this.
@RazrFalcon
Copy link
Owner

Looks good, but we must keep the tests. Removing them is not an option. Not sure what we can do about it.

@adamreichold
Copy link
Collaborator Author

Not sure what we can do about it.

The only thing that comes to my mind is conditional compilation, i.e. have a cfg flag that yields a completely different definition and implementation of Context which still collects tokens. But to be honest, considering that the devirtualization has already reaped most of the available performance benefits, I am not sure if this is worth it. Especially since it negates the main benefit of this change: simplifying the code.

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