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

Make all methods of std::net::Ipv6Addr const #76206

Merged
merged 4 commits into from
Sep 2, 2020
Merged

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Sep 1, 2020

Make the following methods of std::net::Ipv6Addr unstable const under the const_ipv6 feature:

  • segments
  • is_unspecified
  • is_loopback
  • is_global (unstable)
  • is_unique_local
  • is_unicast_link_local_strict
  • is_documentation
  • multicast_scope
  • is_multicast
  • to_ipv4_mapped
  • to_ipv4

This would make all methods of Ipv6Addr const.

Changed the implementation of is_unspecified and is_loopback to use a match instead of ==, all other methods did not require a change.

All these methods are dependent on segments, the current implementation of which requires unstable const_fn_transmute (PR#75085).

Part of #76205

Make the following methods of `std::net::Ipv6Addr` unstable const under the `const_ipv6` feature:
- `segments`
- `is_unspecified`
- `is_loopback`
- `is_global` (unstable)
- `is_unique_local`
- `is_unicast_link_local_strict`
- `is_documentation`
- `multicast_scope`
- `is_multicast`
- `to_ipv4_mapped`
- `to_ipv4`

Changed the implementation of `is_unspecified` and `is_loopback` to use a `match` instead of `==`.

Part of rust-lang#76205
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2020
@jonas-schievink jonas-schievink added A-const-fn T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 1, 2020
@Mark-Simulacrum
Copy link
Member

r? @dtolnay, needs libs FCP

@Mark-Simulacrum
Copy link
Member

Oh, actually, missed that this is just unstable, so r? @ecstatic-morse

@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 1, 2020

Yeah it's unstable for now since segments depends on const_fn_transmute as an optimization. However, interestingly, this also applies to new, which uses #[allow_internal_unstable(const_fn_transmute)], since new was already stabilized as const.

To stabilize these methods as const requires either

  • wait for const_fn_transmute to stabilize
  • temporarily revert PR#75085 (remove the transmute, possibly leads to worse performance)
  • #[allow_internal_unstable(const_fn_transmute)] on segments (like new)

pub fn is_unspecified(&self) -> bool {
self.segments() == [0, 0, 0, 0, 0, 0, 0, 0]
pub const fn is_unspecified(&self) -> bool {
matches!(self.segments(), [0, 0, 0, 0, 0, 0, 0, 0])
Copy link
Contributor

@ecstatic-morse ecstatic-morse Sep 1, 2020

Choose a reason for hiding this comment

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

As a general rule, matches! won't optimize as well as comparison to an array. I don't think the performance of is_unspecified and is_loopback is terribly important for real code, so this is fine for now. It would be good to look into constifying slice equality, making memcmp callable in a const context, or generating better code for array patterns in match statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, which is a shame. I was looking into this for the Ipv4Addr methods as well, most of which can be written very elegantly using matches!. If performance is a concern, the same implementation could be used as in Ipv4Addr::is_global:

u128::from_be_bytes(self.octets()) == 0

Copy link
Contributor

@ecstatic-morse ecstatic-morse Sep 1, 2020

Choose a reason for hiding this comment

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

Yeah, let's do it that way. Instead of comparing with an integer literal, however, you should convert the LOOPBACK and UNSPECIFIED associated constants to a u128 and compare with those.

@ecstatic-morse
Copy link
Contributor

allow_internal_unstable would be fine on segments by the same logic it is okay on new: There is a const-stable alternative implementation. However, I think it's fine to have these be unstably const for a little while.

Ping me once there's tests for these methods and I'll approve this. FWIW, I also find writing "is usable in a const context" tests dull, but c'est la vie.

@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 1, 2020

That's alright. About those tests (also relevant for the other PRs), right now they live under src/test/ui/consts, as that is where const-option.rs already was. However I noticed that there is src/test/ui/consts/std, so I'm going to create the tests for this PR under there. After that, shall I also move the tests from other PRs there, to collect the library interface const tests and separate them from the const implementation tests? Maybe also move the const tests for NonZeroX and the like.

@ecstatic-morse
Copy link
Contributor

Having them live under std seems clearly better, and I would happily sign off on it. OTOH, src/test/ui/consts is already small enough that ./x.py test -- src/test/ui/consts runs pretty quickly, so it's not terribly urgent.

CDirkx and others added 2 commits September 1, 2020 21:05
… from `matches!` to `u128` comparison

Done because `matches!` doesn't optimize well with array comparisons
@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 1, 2020

@ecstatic-morse Tests added :)

@ecstatic-morse
Copy link
Contributor

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 1, 2020

📌 Commit 9afe97c has been approved by ecstatic-morse

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#74880 (Add trailing comma support to matches macro)
 - rust-lang#76074 (Add new `-Z dump-mir-spanview` option)
 - rust-lang#76088 (Add more examples to lexicographic cmp on Iterators.)
 - rust-lang#76099 (Add info about `!` and `impl Trait`)
 - rust-lang#76126 (Use "Fira Sans" for crate list font)
 - rust-lang#76132 (Factor out StmtKind::MacCall fields into `MacCallStmt` struct)
 - rust-lang#76143 (Give a better error message for duplicate built-in macros)
 - rust-lang#76158 (Stabilise link-self-contained option)
 - rust-lang#76201 (Move to intra-doc links for library/core/src/panic.rs)
 - rust-lang#76206 (Make all methods of `std::net::Ipv6Addr` const)
 - rust-lang#76207 (# Move to intra-doc links for library/core/src/clone.rs)
 - rust-lang#76212 (Document lint missing_doc_code_examples is nightly-only)
 - rust-lang#76218 (lexer: Tiny improvement to shebang detection)
 - rust-lang#76221 (Clean up header in `iter` docs for `for` loops)

Failed merges:

r? @ghost
@bors bors merged commit 11ff32f into rust-lang:master Sep 2, 2020
@CDirkx CDirkx deleted the const-ipv6 branch September 3, 2020 21:57
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 24, 2020
Make delegation methods of `std::net::IpAddr` unstably const

Make the following methods of `std::net::IpAddr` unstable const under the `const_ip` feature:
 - `is_unspecified`
 - `is_loopback`
 - `is_global`
 - `is_multicast`

Also adds a test for these methods in a const context.

Possible because these methods delegate to the inner `Ipv4Addr` or `Ipv6Addr`, which were made const ([PR#76205](rust-lang#76142) and [PR#76206](rust-lang#76206)), and the recent stabilization of const control flow.

Part of rust-lang#76205

r? @ecstatic-morse
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2020
Stabilize all stable methods of `Ipv4Addr`, `Ipv6Addr` and `IpAddr` as const

This PR stabilizes all currently stable methods of `Ipv4Addr`, `Ipv6Addr` and `IpAddr` as const.
Tracking issue: rust-lang#76205

`Ipv4Addr` (`const_ipv4`):
 - `octets`
 - `is_loopback`
 - `is_private`
 - `is_link_local`
 - `is_multicast`
 - `is_broadcast`
 - `is_docmentation`
 - `to_ipv6_compatible`
 - `to_ipv6_mapped`

`Ipv6Addr` (`const_ipv6`):
 - `segments`
 - `is_unspecified`
 - `is_loopback`
 - `is_multicast`
 - `to_ipv4`

`IpAddr` (`const_ip`):
 - `is_unspecified`
 - `is_loopback`
 - `is_multicast`

## Motivation
The ip methods seem like prime candidates to be made const: their behavior is defined by an external spec, and based solely on the byte contents of an address. These methods have been made unstable const in the beginning of September, after the necessary const integer arithmetic was stabilized.

There is currently a PR open (rust-lang#78802) to change the internal representation of `IpAddr{4,6}` from `libc` types to a byte array. This does not have any impact on the constness of the methods.

## Implementation
Most of the stabilizations are straightforward, with the exception of `Ipv6Addr::segments`, which uses the unstable feature `const_fn_transmute`. The code could be rewritten to equivalent stable code, but this leads to worse code generation (rust-lang#75085).
This is why `segments` gets marked with `#[rustc_allow_const_fn_unstable(const_fn_transmute)]`, like the already const-stable `Ipv6Addr::new`, the justification being that a const-stable alternative implementation exists rust-lang#76206 (comment).

## Future posibilities
This PR const-stabilizes all currently stable ip methods, however there are also a number of unstable methods under the `ip` feature (rust-lang#27709). These methods are already unstable const. There is a PR open (rust-lang#76098) to stabilize those methods, which could include const-stabilization. However, stabilizing those methods as const is dependent on `Ipv4Addr::octets` and `Ipv6Addr::segments` (covered by this PR).
@cuviper cuviper added this to the 1.48.0 milestone Nov 16, 2023
@RalfJung RalfJung added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants