-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Refactor parsing in bevy_reflect path module #9048
Conversation
a5fa5ea
to
5617e80
Compare
5617e80
to
7779156
Compare
I found out that parsing in "ascii mode" is much better in terms of performance. And there is no reason to not do it (apart from maintainability). What I will do is remove the comment wrt alternative impl and open a performance PR with all the unsafe. |
This is the optimized version I came up with: https://gist.github.com/nicopap/9a1c04bf549914036ea5cc5d6e23269b |
Are you planning to add this optimized version in this PR or opening a followup PR? |
follow up PR |
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.
Looks great! Just two nits and then some other non-blocking comments.
use super::*; | ||
use crate::path::ParsedPath; | ||
|
||
#[test] |
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.
Nit: Not blocking, but while we're here could we add some tests for the other kinds of errors?
Objective
path
module ofbevy_reflect
#8887bevy_reflect/src/path/mod.rs
could also do with some cleanupSolution
parse.rs
module, move all parsing code to this moduleDetailed changes
PathParser
toparse.rs
submoduletoken_to_access
toaccess_following
(yep, goes from 132 lines to 16)parse.rs
file