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

Tracking issue for IP constructors #44582

Closed
jcdyer opened this Issue Sep 15, 2017 · 25 comments

Comments

Projects
None yet
@jcdyer
Contributor

jcdyer commented Sep 15, 2017

Tracking issue for IP address convenience constructors, added in #44395, modified in #52872.

impl Ipv4Addr {
    pub const LOCALHOST: Self = Ipv4Addr::new(127, 0, 0, 1);
    pub const UNSPECIFIED: Self = Ipv4Addr::new(0, 0, 0, 0);
    pub const BROADCAST: Self = Ipv4Addr::new(255, 255, 255, 255);
}

impl Ipv6Addr {
    pub const LOCALHOST: Self = Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1);
    pub const UNSPECIFIED: Self = Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0);
}
@ghost

This comment has been minimized.

ghost commented Oct 7, 2017

Is there any way to implement these as constants?

I personally don't like this kind of trivial function.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Dec 24, 2017

See also #39819

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

I’d also like these to be associated const items instead. This would require at least these APIs to be made const fn:

  • Ipv4Addr::new
  • Ipv6Addr::new
  • std::net::hton
  • u32::to_be
  • u32::swap_bytes

If we’re doing down this road, it seems like many more APIs should also be const fns for consistency, which is starting to be out of scope for this issue.

@rust-lang/libs what do you think?

@sfackler

This comment has been minimized.

Member

sfackler commented Mar 17, 2018

I think I'd prefer these to be associated consts as well. We don't necessarily need to make all of those functions const as well - we can just directly initialize the structures.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

Hmm yes, I suppose we could replace the hton call with two definitions based on target endianness #[cfg].

@Kixunil

This comment has been minimized.

Kixunil commented Mar 18, 2018

Maybe I'm missing something, but aren't IP addresses independent of endianness? In other words, aren't they [u8; 4]?

@sfackler

This comment has been minimized.

Member

sfackler commented Mar 18, 2018

An Ipv4Addr is a wrapper around the libc in_addr type, which has a u32 field that needs to hold the IP in big endian.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 18, 2018

Right. [u8; 4] would be a fine way to store it in big-endian with cross-platform code, but Because Reasons the internal type is ultimately big-endian u32.

@Kixunil

This comment has been minimized.

Kixunil commented Mar 18, 2018

OK, thanks for explaining! An alternative would be to just transmute(), but I'm not sure if transmute() is a const fn.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 18, 2018

I think I’d prefer this over transmute:

        Ipv4Addr {
            inner: c::in_addr {
                // 127.0.0.1
                #[cfg(target_endian = "big")]
                s_addr: 0x7F_00_00_01_u32,
                #[cfg(target_endian = "little")]
                s_addr: 0x01_00_00_7F_u32,
            }
        }

(Either way, with an assert_eq!(Ipv4Addr::LOCALHOST, Ipv4Addr::new(127, 0, 0, 1)) test.)

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 28, 2018

The libs team discussed this and the consensus was to stabilize this as associated constants (in uppercase: LOCALHOST and UNSPECIFIED.)

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Mar 28, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

rfcbot commented Mar 28, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Apr 4, 2018

I started working on changing this to associated constants, but ran into a bit of a roadblock. in6_addr has a non-public parameter, so I'm not sure how to construct an instance of it in a const expression. The only ways I can think of resolving that are to add constant in6_addr instances to libc (at least a zeroed out instance that could then use struct update syntax to update), or make the non-public fields public (which may be problematic if they vary across environments).

@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Apr 4, 2018

hmm, actually it looks like the non-public field is just used for alignment. Maybe now that #[repr(align(...))] is stabilzed the extra field could be removed in favor of #[repr(align(4))]

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Apr 4, 2018

@tmccombs Maybe add const fn new(…) -> Self to those in6_addr structs?

tmccombs added a commit to tmccombs/libc that referenced this issue Apr 5, 2018

Use #[repr(align(32))] instead of private field for in6_addr.
This allows constructiong in6_addr instances as a constant from other
crates.

See rust-lang/rust#44582 (comment)

tmccombs added a commit to tmccombs/libc that referenced this issue Apr 5, 2018

Use #[repr(align(32))] instead of private field for in6_addr.
This allows constructiong in6_addr instances as a constant from other
crates.

See rust-lang/rust#44582 (comment)
@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Apr 5, 2018

Maybe add const fn new(…) -> Self to those in6_addr structs?

For unix at least, in6_addr comes from the libc crate. Which it looks like has to support older versions of rust. So even when const fn is stabilized we can't add it there.

Similarly I don't think I can add #[repr(align(4))] to libc implementations, unless there is a way to use conditional compilation based on whether a feature is stable, or the version of rust, but I don't know how to do that.

@rfcbot

This comment has been minimized.

rfcbot commented Apr 7, 2018

The final comment period is now complete.

@faern

This comment has been minimized.

Contributor

faern commented Jun 1, 2018

I just got PR #51171 merged making the compiler intrinsics for endianess conversion (bswap) a const fn. As soon as that hits beta, and thus the stage0 compiler, I plan on pushing a PR making swap_bytes and to_be const fns on all integers. If this gets accepted, the next step is to make Ipv4Addr::new a const fn as well. If all of this goes through these special IP addresses can easily be made into associated constants for Ipv4Addr.

For Ipv6Addr I don't know yet. It uses mem::zeroed() which is not a const fn.

@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Jun 2, 2018

It uses mem::zeroed() which is not a const fn.

And I don't think it can be, unless it is a compiler intrinsic. And the Ipv4Addr is currently doable (although having a const to_be fn would make it simpler), but the private field for Ipv6Addr is complicated.

@faern

This comment has been minimized.

Contributor

faern commented Jul 31, 2018

I implemented this (and a bit more) in #52872

bors added a commit that referenced this issue Aug 3, 2018

Auto merge of #52872 - faern:use-modern-alignment-libc, r=TimNN
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

cramertj added a commit to cramertj/rust that referenced this issue Aug 3, 2018

Rollup merge of rust-lang#52872 - faern:use-modern-alignment-libc, r=…
…TimNN

Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes rust-lang#44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

bors added a commit that referenced this issue Aug 4, 2018

Auto merge of #52872 - faern:use-modern-alignment-libc, r=TimNN
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

bors added a commit that referenced this issue Aug 5, 2018

Auto merge of #52872 - faern:use-modern-alignment-libc, r=TimNN
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

bors added a commit that referenced this issue Aug 8, 2018

Auto merge of #52872 - faern:use-modern-alignment-libc, r=TimNN
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

bors added a commit that referenced this issue Aug 8, 2018

Auto merge of #52872 - faern:use-modern-alignment-libc, r=TimNN
Make IpvXAddr::new const fns and the well known addresses associated constants

Implements/fixes #44582

I just got a PR towards libc (rust-lang/libc#1044) merged. With the new feature added in that PR it is now possible to create `in6_addr` instances as consts. This enables us to finally make the constructors of the IP structs const fns and to make the localhost/unspecified addresses associated constants, as agreed in the above mentioned tracking issue.

I also added a BROADCAST constant. Personally this is the well known address I tend to need the most often.

@bors bors closed this in #52872 Aug 8, 2018

@faern

This comment has been minimized.

Contributor

faern commented Aug 8, 2018

This should probably not have been closed 🙈 It was me invalidly writing fixes <number> in my PR moving from functions to associated constants.

Anyway. Now that this is merged, all that should be left is to mark the constants as stable I guess.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Aug 8, 2018

Reopening to track stabilization.

@SimonSapin SimonSapin reopened this Aug 8, 2018

@faern

This comment has been minimized.

Contributor

faern commented Aug 8, 2018

I should point out that I also added Ipv4Addr::BROADCAST in the same PR where I converted them from functions to constants. Commit: 7167a06

BROADCAST is added under the same feature gate. So probably good to voice your opinions on that if there are any.

tmccombs added a commit to tmccombs/rust that referenced this issue Aug 9, 2018

@tmccombs

This comment has been minimized.

Contributor

tmccombs commented Aug 9, 2018

I created a PR to stabilize (including BROADCAST) but can change it if that isn't desired.

tmccombs added a commit to tmccombs/rust that referenced this issue Aug 15, 2018

bors added a commit that referenced this issue Aug 20, 2018

Auto merge of #53213 - tmccombs:stable-ipconstructors, r=KodrAus
Stabilize IP associated constants

Fixes #44582

kennytm added a commit to kennytm/rust that referenced this issue Aug 20, 2018

Rollup merge of rust-lang#53213 - tmccombs:stable-ipconstructors, r=K…
…odrAus

Stabilize IP associated constants

Fixes rust-lang#44582

@bors bors closed this in #53213 Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment