Expand size_of docs#44648

Havvy · 2017-09-17T05:18:04Z

This PR does 3 things.

Adds a description of what pointer size means to the primitive pages for usize and isize.
Says the general size of things is not stable from compiler to compiler.
Adds a table of sizes of things that we do guarantee. As this is the first table in the libstd docs, I've included a picture of how that looks.

Havvy · 2017-09-17T05:18:17Z

Havvy · 2017-09-17T05:23:56Z

At the current state of this PR, I've more questions than I have content, so consider this more like an issue than a PR at the moment.

The wording in size_of really isn't that good. I want help on figuring out how it should be worded, especially since it involves stability guarantees, and I don't know how to properly word that. Perhaps we should say the size is only not guaranteed for #[repr(Rust)]?

I don't like the usage of T and TU in the table. The size of a reference changes depending on whether or not its referring to an unsized object, but I don't know how to put that in the table succinctly. Do we just add reference size and point out in the reference module that a reference is pointer size for sized types and twice pointer sized for unsized types?

Are there any other structs that we guarantee the size for? Box<T> perhaps? Wrapper structs?

QuietMisdreavus · 2017-09-17T15:55:24Z

src/libcore/mem.rs

small nit: Option<&UT>

scottmcm · 2017-09-18T02:32:10Z

src/libcore/mem.rs

While you're here, consider mentioning that this is "successive elements in an array with that item type", since layout optimization can (in theory at least) change the layout of "successive items of the same type" in a struct.

scottmcm · 2017-09-18T02:49:08Z

src/libstd/primitive_docs.rs

nit: "one byte" here distracted me, though perhaps only in context of reviewing something about sizes. Perhaps a phrasing like "to reference any location in memory"?

If I change it to this, I have to also describe what a location in memory is, no?

Hmm, *T::offset talks of "allocated object", so maybe use that instead of location? Though I don't know if it's defined anywhere (other than LLVM) either...

scottmcm · 2017-09-18T02:51:34Z

src/libcore/mem.rs

Perhaps mention that it's always a multiple of align_of::<T>(), as well?

I don't know what you mean by this.

scottmcm · 2017-09-18T02:54:59Z

src/libcore/mem.rs

I think the accepted extern types RFC means that there will exist types that are not Sized but are none-the-less thin pointers. And there has been discussion of extra-fat pointers for things like dyn (Debug + Add). So it might be better to not mention this particularly, especially since use cases for relying on it are kinda sketchy.

So basically the size of &T is generally unstable?

scottmcm · 2017-09-18T02:55:28Z

src/libcore/mem.rs

Should probably have *const T and *mut T as well.

scottmcm · 2017-09-18T06:27:27Z

src/libcore/mem.rs

[00:03:45] tidy error: /checkout/src/libcore/mem.rs:182: trailing whitespace

and size_of::<T>() here as well

steveklabnik · 2017-09-18T14:41:00Z

@rust-lang/libs, are you okay with what we are guaranteeing stable here?

sfackler · 2017-09-18T14:53:53Z

They seem reasonable to me.

kennytm · 2017-09-18T14:51:32Z

src/libcore/mem.rs

Typo.

assert_eq!(8, mem::size_of::<f64)()); // ^ should be `>`

As kennytm wrote, there is a typo after f64.

dtolnay · 2017-09-18T15:01:09Z

src/libcore/mem.rs

Could we say "are" instead of "will be", and "has"/"have" instead of "will have" above? The rest of this documentation is written in present tense so "will" makes it seem like those features are not implemented yet.

dtolnay · 2017-09-18T15:09:28Z

src/libcore/mem.rs

I found this a little hard to parse. A skeptical reader would read this as:

Options &T \ / of Box<T> \ / and the pointer \ / have the same size as

instead of

&T Box<T> \ / Options and \ / of the pointer \ / have the same size as

because that sounds more plausible to someone skeptical of zero overhead abstractions.

Is there a way to word this that shows the full types Option<&T> and Option<Box<T>> in the sentence?

Maybe something straightforward like:

The types *T, &T, Box<T>, Option<&T>, and Option<Box<T>> all have the same size. If T is Sized, all of those have the same size as usize.

Yeah, I was sort of floundering with that section. That reads a lot better.

durka · 2017-09-18T15:38:40Z

src/libcore/mem.rs

size_of::<Type>()

As durka wrote, this would be clearer as size_of::<Type>() instead of size_of<Type>().

Also I believe these need to be in backticks or escaped with a backslash. Otherwise markdown will treat it as a <Type> HTML tag.

`Type` | `size_of::<Type>()`

Type | size_of::\<Type>()

durka · 2017-09-18T15:39:26Z

src/libcore/mem.rs

This might be the first place we say *T instead of *const T/*mut T? Likewise, everything here about &T applies to &mut T.

durka · 2017-09-18T15:43:52Z

src/libcore/mem.rs

error: expected mut or const in raw pointer type (use `*mut T` or `*const T` as appropriate)

durka · 2017-09-18T15:44:14Z

src/libcore/mem.rs

typo Box<32>

dtolnay · 2017-09-18T19:37:47Z

One more type we guarantee is extern "C" fn(_) -> _ and Option<extern "C" fn(_) -> _> which are guaranteed to be the same size as _ (*)(_) in C. I don't know whether we also guarantee the same for extern "Rust" or whether the size is guaranteed to be the same as usize.

matthewjasper · 2017-09-19T12:28:33Z

src/libcore/mem.rs

; to separate i32 from the length.

I thought I was running the doctests; I actually wasn't.

Havvy · 2017-09-20T02:24:20Z

All nits should be fixed. I don't have dtolnay's thing here. I'm wondering if the null pointer optimization for Option-shaped enums is actually stable, and if so, we could just say that, and then point out what the common nullable pointer types are, including fn pointers.

I'm avoiding talking about #[repr(C)] or any other representation here, since I want to do that in its own PR if we want to talk about representations at all in size_of.

I can also squash all the commits into one if you want.

scottmcm · 2017-09-20T02:42:17Z

I like what you have here; the Option<&_> and Option<Box<_>> cases are certainly the critical ones.

Details about enum size is probably mixed up in #27730 & rust-lang/rfcs#1230, and fine to leave for later.

dtolnay · 2017-09-20T04:03:26Z

src/libcore/mem.rs

[T; n] should be in backticks.

dtolnay · 2017-09-20T04:04:24Z

src/libcore/mem.rs

As durka wrote, this would be clearer as size_of::<Type>() instead of size_of<Type>().

dtolnay · 2017-09-20T04:05:11Z

src/libcore/mem.rs

The rest of this documentation is in present tense so "will have" makes it sound like this part is not implemented yet. Plain "have" should be sufficient I think.

dtolnay · 2017-09-20T04:06:30Z

src/libcore/mem.rs

As kennytm wrote, there is a typo after f64.

dtolnay · 2017-09-20T04:07:04Z

src/libcore/mem.rs

This should say [i32; 3] instead of [i32, 3].

dtolnay · 2017-09-20T04:14:30Z

r=me after the last few typos are addressed. I agree we don't need to include fn and repr(C) in this PR.

Havvy · 2017-09-21T04:15:59Z

Finally figured out how to get rustdoc tests to compile for this. Items in libstd from the libcore facade don't actually get tested from ./x.py test src/libstd and I have to do ./x.py test src/libcore instead.

Also rebased everything into a single commit to avoid polluting the commit log with typo fixing and whatnot.

dtolnay · 2017-09-21T04:41:13Z

@bors r+

bors · 2017-09-21T04:41:14Z

📌 Commit 548686f has been approved by dtolnay

arielb1 · 2017-09-21T11:14:55Z

@bors rollup

Expand size_of docs This PR does 3 things. 1. Adds a description of what pointer size means to the primitive pages for usize and isize. 2. Says the general size of things is not stable from compiler to compiler. 3. Adds a table of sizes of things that we do guarantee. As this is the first table in the libstd docs, I've included a picture of how that looks. ![](https://i.imgur.com/YZ6IChH.png?1)

Rollup of 14 pull requests - Successful merges: #44554, #44648, #44658, #44712, #44717, #44726, #44745, #44746, #44749, #44759, #44770, #44773, #44776, #44778 - Failed merges:

rust-highfive assigned steveklabnik Sep 17, 2017

QuietMisdreavus reviewed Sep 17, 2017

View reviewed changes

src/libcore/mem.rs Outdated

Copy link

Contributor

QuietMisdreavus Sep 17, 2017

Choose a reason for hiding this comment

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

small nit: Option<&UT>

scottmcm reviewed Sep 18, 2017

View reviewed changes

kennytm reviewed Sep 18, 2017

View reviewed changes

dtolnay reviewed Sep 18, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 18, 2017

durka approved these changes Sep 18, 2017

View reviewed changes

durka reviewed Sep 18, 2017

View reviewed changes

src/libcore/mem.rs Outdated

Copy link

Contributor

durka Sep 18, 2017

Choose a reason for hiding this comment

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

error: expected mut or const in raw pointer type (use `*mut T` or `*const T` as appropriate)

durka reviewed Sep 18, 2017

View reviewed changes

src/libcore/mem.rs Outdated

Copy link

Contributor

durka Sep 18, 2017

Choose a reason for hiding this comment

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

typo Box<32>

matthewjasper reviewed Sep 19, 2017

View reviewed changes

Havvy force-pushed the doc-size_of branch from 20a9149 to 2983d5d Compare September 20, 2017 02:17

dtolnay requested changes Sep 20, 2017

View reviewed changes

Document stable size_of primitives and pointer size guarantees

548686f

Havvy force-pushed the doc-size_of branch from b1a27eb to 548686f Compare September 21, 2017 04:11

dtolnay approved these changes Sep 21, 2017

View reviewed changes

GuillaumeGomez mentioned this pull request Sep 21, 2017

Rollup of 7 pull requests #44761

Closed

frewsxcv mentioned this pull request Sep 23, 2017

Rollup of 14 pull requests #44784

Merged

bors merged commit 548686f into rust-lang:master Sep 23, 2017

Havvy mentioned this pull request Nov 22, 2017

Document the size of bool #46156

Merged

dtolnay assigned dtolnay and unassigned steveklabnik Mar 24, 2024

Uh oh!

Conversation

Havvy commented Sep 17, 2017

Uh oh!

Havvy commented Sep 17, 2017

Uh oh!

Havvy commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklabnik commented Sep 18, 2017

Uh oh!

sfackler commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Havvy commented Sep 20, 2017

Uh oh!

scottmcm commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Havvy commented Sep 17, 2017 •

edited

Loading

scottmcm Sep 18, 2017 •

edited

Loading