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

Add missing examples for Ipv6Addr #37859

Merged
merged 2 commits into from
Dec 4, 2016
Merged

Conversation

GuillaumeGomez
Copy link
Member

@frewsxcv
Copy link
Member

@bors r+ rollup

@frewsxcv
Copy link
Member

Travis error is unrelated.

/// use std::net::Ipv6Addr;
///
/// assert_eq!(Ipv6Addr::new(65280, 0, 0, 0, 0, 0, 0, 0).octets(),
/// [65280, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
Copy link
Member

Choose a reason for hiding this comment

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

This should be [255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0].

Copy link
Member

Choose a reason for hiding this comment

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

Yep, good catch!

@frewsxcv
Copy link
Member

@bors r-

@GuillaumeGomez
Copy link
Member Author

Updated.

@bors: delegate=frewsxcv

@bors
Copy link
Contributor

bors commented Nov 18, 2016

✌️ @frewsxcv can now approve this pull request

@frewsxcv
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 18, 2016

📌 Commit f6c6ad9 has been approved by frewsxcv

/// ```
/// use std::net::Ipv6Addr;
///
/// let addr = Ipv6Addr::new(0, 0, 0, 0, 0, 65535, 49152, 767);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad example, should use hex instead, since that’s the representation used most commonly.

Compare:

# for `2002:c000:2ff::` aka `::ffff:192.0.2.255`
let addr = Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0xc00a, 0x2ff);

Using an 6to4 address is not a good idea either.

/// ```
/// use std::net::Ipv6Addr;
///
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 65535, 49152, 767).is_unspecified(), false);
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, use hex please.

/// fn main() {
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 65535, 49152, 767).is_global(), true);
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1).is_global(), false);
/// assert_eq!(Ipv6Addr::new(0, 0, 457, 0, 0, 45000, 0, 1).is_global(), true);
Copy link
Member

Choose a reason for hiding this comment

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

Compare:

# for ::1c9:0:0:afc8:0:1
Ipv6Addr::new(0, 0, 0x1c9, 0, 0, 0xafc8, 0, 1)

@GuillaumeGomez
Copy link
Member Author

@bors: r-

@GuillaumeGomez
Copy link
Member Author

Updated.

/// fn main() {
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0xc00a, 0x2ff).is_global(), true);
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0x1).is_global(), false);
/// assert_eq!(Ipv6Addr::new(0, 0, 0x1C9, 0, 0, 0xafc8, 0, 0x1).is_global(), true);
Copy link
Member

Choose a reason for hiding this comment

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

C in 0x1c9 is uppercase, despite of everything else being lowercase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

///
/// assert_eq!(Ipv6Addr::new(0xff00, 0, 0, 0, 0, 0, 0, 0).to_ipv4(), None);
/// assert_eq!(Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0xc00a, 0x2ff).to_ipv4(),
/// Some(Ipv4Addr::new(127, 0, 0, 1)));
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn’t look right. 127.0.0.1 is ::ffff:7f00:0001. What you have there now is 192.10.2.255.

Copy link
Member Author

Choose a reason for hiding this comment

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

Arf, indeed. Bad conversion!

Copy link
Member

Choose a reason for hiding this comment

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

Probably also a good idea to show that e.g. ::1 converts to a 0.0.0.1 rather than None (i.e. it does both decimal and 4to6 conversions)

@GuillaumeGomez
Copy link
Member Author

Updated.

@BurntSushi BurntSushi added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Nov 21, 2016
@nagisa
Copy link
Member

nagisa commented Nov 22, 2016

r=me

I also wrote a comment like this:

Probably also a good idea to show that e.g. ::1 converts to a 0.0.0.1 rather than None (i.e. it does both decimal and 4to6 conversions)

which got immediately hidden by a commit, but even without that the PR looks fine to me now.

@GuillaumeGomez
Copy link
Member Author

Then let's let @frewsxcv decide. :)

@frewsxcv
Copy link
Member

/checkout/src/libstd/net/ip.rs:669: line longer than 100 chars
/checkout/src/libstd/net/ip.rs:684: line longer than 100 chars

I think it would be good to include @nagisa's example, but I don't feel strongly.

r=me with these changes.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Nov 22, 2016
@GuillaumeGomez
Copy link
Member Author

@bors: r=nagisa

Thanks to both @frewsxcv and @nagisa for this review. :)

@bors
Copy link
Contributor

bors commented Nov 22, 2016

📌 Commit 57be2d9 has been approved by nagisa

@GuillaumeGomez
Copy link
Member Author

@bors: r-

bors added a commit that referenced this pull request Nov 23, 2016
Rollup of 7 pull requests

- Successful merges: #37442, #37760, #37836, #37851, #37859, #37913, #37925
- Failed merges:
@GuillaumeGomez
Copy link
Member Author

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Nov 29, 2016

📌 Commit a5049f7 has been approved by GuillaumeGomez

@GuillaumeGomez
Copy link
Member Author

@bors: r=nagisa

@bors
Copy link
Contributor

bors commented Nov 29, 2016

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Nov 29, 2016

📌 Commit a5049f7 has been approved by nagisa

dns2utf8 added a commit to dns2utf8/rust that referenced this pull request Dec 1, 2016
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 3, 2016
bors added a commit that referenced this pull request Dec 3, 2016
Rollup of 15 pull requests

- Successful merges: #37859, #37919, #38020, #38028, #38029, #38065, #38073, #38077, #38089, #38090, #38096, #38112, #38113, #38130, #38141
- Failed merges:
@bors bors merged commit a5049f7 into rust-lang:master Dec 4, 2016
@GuillaumeGomez GuillaumeGomez deleted the net_examples branch December 4, 2016 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants