Document when types have OS-dependent sizes #48932

Phlosioneer · 2018-03-11T11:55:51Z

As per issue #43601, types that can change size depending on the
target operating system should say so in their documentation.

I used this template when adding doc comments:

The size of a(n) <name> struct may vary depending on the target
operating system, and may change between Rust releases.

For enums, I used "instance" instead of "struct".

I added documentation to these types:

- std::time::Instant						(contains sys::time::Instant)
- std::time::SystemTime						(contains sys::time::SystemTime)

- std::io::StdinRaw							(contains sys::stdio::Stdin)
- std::io::StdoutRaw						(contains sys::stdio::Stdout)
- std::io::Stderr							(contains sys::stdio::Stderr)

- std::net::addr::SocketAddrV4				(contains sys::net::netc::sockaddr_in)
- std::net::addr::SocketAddrV6				(contains sys::net::netc::sockaddr_in6)
- std::net::addr::SocketAddr				(contains std::net::addr::SocketAddrV4 and SocketAddrV6)
- std::net::ip::Ipv4Addr					(contains sys::net::netc::in_addr)
- std::net::ip::Ipv6Addr					(contains sys::net::netc::in6_addr)
- std::net::ip::IpAddr						(contains std::net::ip::Ipv4Addr and Ipv6Addr)

I also found that these types varied in size; however, I don't think they need documentation, as it's already fairly obvious that they change based on different OS's:

- std::fs::DirBuilder						(contains sys::fs::DirBuilder)
- std::fs::FileType							(contains sys::fs::FileType)
- std::fs::Permissions						(contains sys::fs::FilePermissions)
- std::fs::OpenOptions						(contains sys::fs::OpenOptions)
- std::fs::DirEntry							(contains sys::fs::DirEntry)
- std::fs::ReadDir							(contains sys::fs::ReadDir)
- std::fs::Metadata							(contains sys::fs::FileAttr)
- std::fs::File								(contains sys::fs::File)

- std::process::Child						(contains sys::process::Process)
- std::process::ChildStdin					(contains sys::process::AnonPipe)
- std::process::ChildStdout					(contains sys::process::AnonPipe)
- std::process::ChildStderr					(contains sys::process::AnonPipe)
- std::process::Command						(contains sys::process::Command)
- std::process::Stdio						(contains sys::process::Stdio)
- std::process::ExitStatus					(contains sys::process::ExitStatus)
- std::process::Output						(contains std::process::ExitStatus)

- std::sys_common::condvar::Condvar			(contains sys::condvar::Condvar)
- std::sys_common::mutex::Mutex				(contains sys::mutex::Mutex)
- std::sys_common::net::LookupHost			(contains sys::net::netc::addrinfo)
- std::sys_common::net::TcpStream			(contains sys::net::Socket)
- std::sys_common::net::TcpListener			(contains sys::net::Socket)
- std::sys_common::net::UdpSocket			(contains sys::net::Socket)
- std::sys_common::remutex::ReentrantMutex	(contains sys::mutex::ReentrantMutex)
- std::sys_common::rwlock::RWLock			(contains sys::rwlock::RWLock)
- std::sys_common::thread_local::Key		(contains sys::thread_local::Key)

Maybe we should just put a comment about the size of structs in the module-level docs for fs, process, and sys_common?

If anyone can think of other types that can change size, comment below. I'm also open to changing the wording.

closes #43601.

As per issue rust-lang#43601, types that can change size depending on the target operating system should say so in their documentation. I used this template when adding doc comments: The size of a(n) <name> struct may vary depending on the target operating system, and may change between Rust releases. For enums, I used "instance" instead of "struct".

rust-highfive · 2018-03-11T11:55:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

sfackler · 2018-03-11T17:03:40Z

There are like 3 types that can't change size between releases so I wouldn't include that bit.

Phlosioneer · 2018-03-13T01:32:15Z

I pushed a new commit; re-review?

Phlosioneer · 2018-03-16T19:29:49Z

@KodrAus re-review? This is just a small documentation addition... If you're busy, we can reassign it to someone else, I'd just like to make progress of some kind.

KodrAus

Thanks @Phlosioneer! Just left some small nitpicks.

KodrAus · 2018-03-16T23:35:06Z

src/libstd/net/addr.rs

@@ -28,6 +28,9 @@ use slice;
 /// as possibly some version-dependent additional information. See [`SocketAddrV4`]'s and
 /// [`SocketAddrV6`]'s respective documentation for more details.
 ///
+/// The size of a SocketAddr instance may vary depending on the target operating


nit: Let's wrap SocketAddr in backticks here.

KodrAus · 2018-03-16T23:35:50Z

src/libstd/io/stdio.rs

@@ -30,18 +30,27 @@ thread_local! {
 ///
 /// This handle is not synchronized or buffered in any fashion. Constructed via
 /// the `std::io::stdio::stdin_raw` function.
+///
+/// The size of a StdinRaw struct may vary depending on the target operating


Since these types aren't publicly exported do you think we need to mention their size variance?

Oh, if it's not public then yeah, that can be omitted.

Phlosioneer · 2018-03-20T11:38:48Z

@KodrAus Addressed your concerns, next time you get a chance to review.

KodrAus

Thanks @Phlosioneer! Just a few more backticks to add, then I think this will be good to go.

KodrAus · 2018-03-20T22:50:40Z

src/libstd/net/addr.rs

@@ -61,6 +64,9 @@ pub enum SocketAddr {
 ///
 /// See [`SocketAddr`] for a type encompassing both IPv4 and IPv6 socket addresses.
 ///
+/// The size of a SocketAddrV4 struct may vary depending on the target operating


Let's put some backticks on SocketAddrV4 here too.

KodrAus · 2018-03-20T22:50:48Z

src/libstd/net/addr.rs

@@ -88,6 +94,9 @@ pub struct SocketAddrV4 { inner: c::sockaddr_in }
 ///
 /// See [`SocketAddr`] for a type encompassing both IPv4 and IPv6 socket addresses.
 ///
+/// The size of a SocketAddrV6 struct may vary depending on the target operating


Let's put some backticks on SocketAddrV6 here too.

KodrAus · 2018-03-20T22:50:56Z

src/libstd/net/ip.rs

@@ -26,6 +26,9 @@ use sys_common::{AsInner, FromInner};
 /// This enum can contain either an [`Ipv4Addr`] or an [`Ipv6Addr`], see their
 /// respective documentation for more details.
 ///
+/// The size of an IpAddr instance may vary depending on the target operating


Let's put some backticks on IpAddr here too.

KodrAus · 2018-03-20T22:51:30Z

src/libstd/net/ip.rs

@@ -61,6 +64,9 @@ pub enum IpAddr {
 ///
 /// See [`IpAddr`] for a type encompassing both IPv4 and IPv6 addresses.
 ///
+/// The size of an Ipv4Addr struct may vary depending on the target operating


Let's put some backticks on Ipv4Addr here too.

KodrAus · 2018-03-20T22:51:37Z

src/libstd/net/ip.rs

@@ -93,6 +99,9 @@ pub struct Ipv4Addr {
 ///
 /// See [`IpAddr`] for a type encompassing both IPv4 and IPv6 addresses.
 ///
+/// The size of an Ipv6Addr struct may vary depending on the target operating


Let's put some backticks on Ipv6Addr here too.

KodrAus · 2018-03-20T22:51:47Z

src/libstd/time.rs

@@ -49,6 +49,9 @@ pub use core::time::Duration;
 /// allows measuring the duration between two instants (or comparing two
 /// instants).
 ///
+/// The size of an Instant struct may vary depending on the target operating


Let's put some backticks on Instant here too.

KodrAus · 2018-03-20T22:51:56Z

src/libstd/time.rs

@@ -88,6 +91,9 @@ pub struct Instant(time::Instant);
 /// fixed point in time, a `SystemTime` can be converted to a human-readable time,
 /// or perhaps some other string representation.
 ///
+/// The size of a SystemTime struct may vary depending on the target operating


Let's put some backticks on SystemTime here too.

Phlosioneer · 2018-03-25T03:42:32Z

Updated, @KodrAus

KodrAus · 2018-03-25T22:05:45Z

Thanks @Phlosioneer!

@bors r+ rollup

bors · 2018-03-25T22:05:46Z

📌 Commit efd0442 has been approved by KodrAus

…ize, r=KodrAus Document when types have OS-dependent sizes As per issue rust-lang#43601, types that can change size depending on the target operating system should say so in their documentation. I used this template when adding doc comments: ``` The size of a(n) <name> struct may vary depending on the target operating system, and may change between Rust releases. ``` For enums, I used "instance" instead of "struct". I added documentation to these types: ``` - std::time::Instant (contains sys::time::Instant) - std::time::SystemTime (contains sys::time::SystemTime) - std::io::StdinRaw (contains sys::stdio::Stdin) - std::io::StdoutRaw (contains sys::stdio::Stdout) - std::io::Stderr (contains sys::stdio::Stderr) - std::net::addr::SocketAddrV4 (contains sys::net::netc::sockaddr_in) - std::net::addr::SocketAddrV6 (contains sys::net::netc::sockaddr_in6) - std::net::addr::SocketAddr (contains std::net::addr::SocketAddrV4 and SocketAddrV6) - std::net::ip::Ipv4Addr (contains sys::net::netc::in_addr) - std::net::ip::Ipv6Addr (contains sys::net::netc::in6_addr) - std::net::ip::IpAddr (contains std::net::ip::Ipv4Addr and Ipv6Addr) ``` I also found that these types varied in size; however, I don't think they need documentation, as it's already fairly obvious that they change based on different OS's: ``` - std::fs::DirBuilder (contains sys::fs::DirBuilder) - std::fs::FileType (contains sys::fs::FileType) - std::fs::Permissions (contains sys::fs::FilePermissions) - std::fs::OpenOptions (contains sys::fs::OpenOptions) - std::fs::DirEntry (contains sys::fs::DirEntry) - std::fs::ReadDir (contains sys::fs::ReadDir) - std::fs::Metadata (contains sys::fs::FileAttr) - std::fs::File (contains sys::fs::File) - std::process::Child (contains sys::process::Process) - std::process::ChildStdin (contains sys::process::AnonPipe) - std::process::ChildStdout (contains sys::process::AnonPipe) - std::process::ChildStderr (contains sys::process::AnonPipe) - std::process::Command (contains sys::process::Command) - std::process::Stdio (contains sys::process::Stdio) - std::process::ExitStatus (contains sys::process::ExitStatus) - std::process::Output (contains std::process::ExitStatus) - std::sys_common::condvar::Condvar (contains sys::condvar::Condvar) - std::sys_common::mutex::Mutex (contains sys::mutex::Mutex) - std::sys_common::net::LookupHost (contains sys::net::netc::addrinfo) - std::sys_common::net::TcpStream (contains sys::net::Socket) - std::sys_common::net::TcpListener (contains sys::net::Socket) - std::sys_common::net::UdpSocket (contains sys::net::Socket) - std::sys_common::remutex::ReentrantMutex (contains sys::mutex::ReentrantMutex) - std::sys_common::rwlock::RWLock (contains sys::rwlock::RWLock) - std::sys_common::thread_local::Key (contains sys::thread_local::Key) ``` Maybe we should just put a comment about the size of structs in the module-level docs for `fs`, `process`, and `sys_common`? If anyone can think of other types that can change size, comment below. I'm also open to changing the wording. closes rust-lang#43601.

Rollup of 7 pull requests - Successful merges: #48693, #48932, #49103, #49170, #49187, #49346, #49353 - Failed merges:

rust-highfive assigned KodrAus Mar 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2018

Remove "and may change between Rust releases"

e63b1a0

KodrAus reviewed Mar 16, 2018

View reviewed changes

Remove StdioRaw doc additions, add backticks

1af952c

KodrAus reviewed Mar 20, 2018

View reviewed changes

Add backticks

efd0442

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 Mar 25, 2018

kennytm mentioned this pull request Mar 26, 2018

Rollup of 8 pull requests #49374

Closed

TimNN mentioned this pull request Mar 26, 2018

Rollup of 7 pull requests #49379

Merged

bors added a commit that referenced this pull request Mar 26, 2018

Auto merge of #49379 - TimNN:rollup, r=TimNN

ab8b961

Rollup of 7 pull requests - Successful merges: #48693, #48932, #49103, #49170, #49187, #49346, #49353 - Failed merges:

bors merged commit efd0442 into rust-lang:master Mar 26, 2018

svartalf mentioned this pull request Jun 8, 2020

Document that many heim types have OS-dependent sizes heim-rs/heim#245

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document when types have OS-dependent sizes #48932

Document when types have OS-dependent sizes #48932

Phlosioneer commented Mar 11, 2018

rust-highfive commented Mar 11, 2018

sfackler commented Mar 11, 2018

Phlosioneer commented Mar 13, 2018

Phlosioneer commented Mar 16, 2018 •

edited

Loading

KodrAus left a comment

KodrAus Mar 16, 2018

KodrAus Mar 16, 2018

Phlosioneer Mar 20, 2018

Phlosioneer commented Mar 20, 2018

KodrAus left a comment

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

KodrAus Mar 20, 2018

Phlosioneer commented Mar 25, 2018

KodrAus commented Mar 25, 2018

bors commented Mar 25, 2018

Document when types have OS-dependent sizes #48932

Document when types have OS-dependent sizes #48932

Conversation

Phlosioneer commented Mar 11, 2018

rust-highfive commented Mar 11, 2018

sfackler commented Mar 11, 2018

Phlosioneer commented Mar 13, 2018

Phlosioneer commented Mar 16, 2018 • edited Loading

KodrAus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phlosioneer commented Mar 20, 2018

KodrAus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phlosioneer commented Mar 25, 2018

KodrAus commented Mar 25, 2018

bors commented Mar 25, 2018

Phlosioneer commented Mar 16, 2018 •

edited

Loading