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

perf: change ports type to Option<NonZeroU16> #931

Closed
wants to merge 1 commit into from

Conversation

DonIsaac
Copy link

Proposal implementation for #930

I've left the public API unchanged; all public port getter/setters still deal with u16s.

@valenting
Copy link
Collaborator

Doesn't this make it impossible to parse http://example.org:0/ ?
That's still a valid URL according to the URL standard.

@DonIsaac
Copy link
Author

Yes, it is a valid port. It appears to be used for some use cases in the wild. I'm wondering if/how we could get this memory layout improvement without impacting users that need port 0. Maybe a feature flag?

@valenting
Copy link
Collaborator

I don't think this is possible for port, but I encourage you to explore options for

    query_start: Option<u32>,    // Before '?', unlike Position::QueryStart
    fragment_start: Option<u32>, // Before '#', unlike Position::FragmentStart

If I'm not mistaken, it should be impossible for them to start at 0.

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.

None yet

2 participants