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

no_std support #7

Merged
merged 10 commits into from
Mar 11, 2023
Merged

no_std support #7

merged 10 commits into from
Mar 11, 2023

Conversation

jelacious
Copy link
Contributor

@jelacious jelacious commented Mar 3, 2023

This PR adds no_std support to the crate by replacing all explicit uses of std with core or alloc. Thus, closes #6.

I removed all Error implementations from custom errors, since the Error trait still hasn't landed in core yet, see 103765.

I have replaced the std::net::Ipv{4,6}Addr structs with very bare-bones versions based on the ones in the standard library. Their implementations might leave something to be desired, and I am open to suggestions on if even having them is necessary. One could replace all instances of their use with just some byte arrays representing the ip addresses, feature-gate them or just leave out them all together.

I have not added a feature-gate for them for the moment, but if desired it is easily done.

This is a fairly brute-force-y way of doing things, so it exposes some extra things as part of the API and may cause some breakage if people are relying on the std::net IP structs.

@yescallop
Copy link
Owner

Thank you for the nice work!

As for replacing Ipv{4,6}Addr with bare-bone versions, I notice that a PR that moves these structs to core (rust-lang/rust#104265) was merged a few days ago and will hit stable on about April 20 with the release of Rust 1.69.0. I think it might be better to wait until that time and then follow up the changes instead.

Also, would you place the Error implementations behind a default feature gate named std so that this is not a breaking change?

@jelacious
Copy link
Contributor Author

jelacious commented Mar 3, 2023

As for replacing Ipv{4,6}Addr with bare-bone versions, I notice that a PR that moves these structs to core (rust-lang/rust#104265) was merged a few days ago and will hit stable on about April 20 with the release of Rust 1.69.0. I think it might be better to wait until that time and then follow up the changes instead.

Oh, I totally missed this. That is fantastic! I will remove my bare-bones stuff and replace them with just use core::net, so when the release rolls around it should just work.

Also, would you place the Error implementations behind a default feature gate named std so that this is not a breaking change?

Absolutely. I'll add the flag where needed.

@jelacious
Copy link
Contributor Author

It is currently in a "broken" state, since we are waiting for the next stable release. I made sure it worked using nightly. If you want to test right now use nightly with the flag #![feature(ip_in_core)].

@yescallop
Copy link
Owner

Thanks for the adjustments! I've just added std as a default feature and bumped the MSRV. I'll merge this PR as soon as Rust 1.69.0 is released and publish a new release of the crate.

@yescallop
Copy link
Owner

Wait a minute. I seem to have mistakenly assumed that the ip_in_core feature would be stable as soon as Rust 1.69.0 is released, from the fact that I saw a milestone of 1.69.0 on the PR page. I'm now afraid this isn't the case.

IUUC, the ip_in_core feature is basically the same as error_in_core, which isn't stablized yet. This means we would need to again consider another approach such as the previous one you had taken, which I guess may require a breaking change.

I should've asked this in the very beginning, but I wonder what your use case of fluent-uri in a no_std environment is like? Would you need to have an IP address parsed in such a case? If no, then it is possible that we remove the address fields in HostData::{Ipv4, Ipv6} and mark these variants as non-exhaustive when std feature is disabled.

@jelacious
Copy link
Contributor Author

jelacious commented Mar 6, 2023

Wait a minute. I seem to have mistakenly assumed that the ip_in_core feature would be stable as soon as Rust 1.69.0 is released, from the fact that I saw a milestone of 1.69.0 on the PR page. I'm now afraid this isn't the case.

Oof, yes. It seems like #108443 is what is keeping track of the stabilization progress. Well, at least we know that sometime in the future the entire crate can be no_std without issue. :P

I should've asked this in the very beginning, but I wonder what your use case of fluent-uri in a no_std environment is like? Would you need to have an IP address parsed in such a case? If no, then it is possible that we remove the address fields in HostData::{Ipv4, Ipv6} and mark these variants as non-exhaustive when std feature is disabled.

I can't believe I didn't think of just removing the address fields. 😖

My use case would be parsing URIs on embedded devices which don't have an OS, and as such cannot use std.

Would removing the IP address fields make the parser not be able to parse IP addresses? I could easily just feature-gate the two fields behind the std feature and write a note in the documentation about them not being there if the std feature flag is not enabled.

@yescallop
Copy link
Owner

Would removing the IP address fields make the parser not be able to parse IP addresses?

It wouldn't, so long as we retain all the IP address parsing code except the part that turns an IP address into Ip{v4, v6}Addr.

By the way, I've already incorporated the changes that remove the fields in the branch 0.2, along with some other changes that might not be very soon published. In this case, would you prefer to have the no_std support published in a 0.1.4 release? You can easily port them into this PR and I'll merge it at once.

@jelacious
Copy link
Contributor Author

I have to the best of my ability ported changes that should not break anything. You may want to double-check so I did not break any public-facing stuff. I included some of the small changes you did to the parser.

I left the features as they were since removing them would be breaking changes.

@yescallop yescallop merged commit dcaceba into yescallop:main Mar 11, 2023
@yescallop
Copy link
Owner

Thanks for the pull request! I have published a new release of version 0.1.4.

@jelacious jelacious deleted the no_std_ipaddr branch March 13, 2023 09:44
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.

no_std support
2 participants