Document repr packed(N).#553

ehuss · 2019-03-25T05:06:42Z

Closes #483

I feel like this is lacking, but I'm not sure what else to add.

Centril

My overall sense is that the text added here would be more succinctly described in a few equations, but I think this is mostly good for now.

src/type-layout.md

Centril · 2019-03-25T06:41:43Z

src/type-layout.md

-It modifies the representation (either the default or `C`) by removing any
-padding bytes and forcing the alignment of the type to `1`.
+The `packed` representation sets the alignment to the smaller of the specified
+packing and the current alignment (either default or `C`). The alignments of


Here the "default" alignment is referenced but above "primitive alignment" is used... we should pick one phrasing.

Get rid of both "current' and "primitive". Get rid of the parenthetical. Instead spell it out explicitly that if the type has the default or C representation, and the alignment from that representation is smaller, it will use that alignment.

Centril · 2019-03-25T06:43:03Z

src/type-layout.md

-It modifies the representation (either the default or `C`) by removing any
-padding bytes and forcing the alignment of the type to `1`.
+The `packed` representation sets the alignment to the smaller of the specified
+packing and the current alignment (either default or `C`). The alignments of


"or C" is not super clear wrt. referring to C-the-language

Co-Authored-By: ehuss <eric@huss.org>

ehuss · 2019-03-25T23:20:01Z

I added an experimental commit to propose a different way of documenting these repr instructions. I think it more accurately captures how it works. That is, there are some basic "representations", and packed and align just alter them. In the relevant cases (on stable) there are only two — C and default. I think it makes it clearer about what can be "combined".

I do have some concerns and doubts:

This introduces new terminology ("modifier") that I made up.
The descriptions between align and packed have a lot of duplication.
I think that align and packed force the alignment to the given value. However, I'm not certain. The parsing code calls them "hints" in the error messages, but that terminology is not used elsewhere. I'm fairly sure that align is forced here and packed is forced here. This 1000 line function is a bit hard to follow.
- Related: I think that packed has a floor of the i8 alignment here. This is always 8-bits on all platforms rustc builds on that I can find. I tried building with a custom target where it was 16-bits, but libcore fails to build with i8:16:32 alignment (and I need the align_of function to test my hypothesis). I'm not sure if this floor should be documented, or if Rust should always assume this to be "1"

src/type-layout.md

Centril · 2019-03-26T01:16:18Z

src/type-layout.md

 The possible representations for a type are the default representation, `C`,
-the primitive representations, `packed`, and `transparent`. Multiple
-representations can be applied to a single type.
+the primitive representations, and `transparent`.


Itemize ^

Also make it clear what the "primitive representations" entail (by itemizing all the primitive reprs)

src/type-layout.md

Centril · 2019-03-26T01:34:34Z

src/type-layout.md

+`#[repr(align(x))]`. The alignment value must be a power of two from 1 up to
+2<sup>29</sup>.
+
+The `align` modifier raises the type's alignment to the specified alignment.


This is a partial repetition of what you said in the leading paragraph.

Centril · 2019-03-26T01:35:46Z

src/type-layout.md

+2<sup>29</sup>.
+
+The `align` modifier raises the type's alignment to the specified alignment.
+If the specified alignment is less than the alignment of the type without the


I believe this concept of "without the ** modifier" is referred to as the "natural **", for example "natural alignment". I think it would be good to introduce such terms earlier (when we first start to speak about alignment) and then use them.

Centril · 2019-03-26T01:38:43Z

src/type-layout.md

+The packing value is specified as an integer parameter in the form of
+`#[repr(packed(x))]`. If no value is given, as in `#[repr(packed)]`, then the
+packing value is 1. The packing value must be a power of two from 1 up to
+2<sup>29</sup>.


If you extract out the wording about the default it is the same as the paragraph for align. So extract these two out into a common notion repr_value or something and then reference that one.

src/type-layout.md

Co-Authored-By: ehuss <eric@huss.org>

ehuss · 2019-03-26T18:34:36Z

I attempted to reduce some of the duplication. It still seems a little awkward, though.

Centril · 2019-03-27T05:00:11Z

I think this is strict improvement for now. Thanks @ehuss.
Feel free to merge at will :)

retep998 · 2019-04-06T18:03:21Z

Technically it would be more accurate to say packed lowers the alignment of the fields in the struct, rather than the struct itself. But since the alignment of the struct itself is determined from the maximum alignment of the fields, the struct alignment effectively decreases as well. align meanwhile increases the alignment of the struct but not its fields.

Centril · 2019-04-06T18:46:38Z

@retep998 Could you file a clarification PR?

Update books ## nomicon 1 commits in f1ff93b66844493a7b03101c7df66ac958c62418..c02e0e7754a76886e55b976a3a4fac20100cd35d 2019-02-26 13:37:28 -0500 to 2019-03-25 16:52:56 -0400 - dropck: The drop order is now defined (rust-lang/nomicon#113) ## reference 3 commits in 27ad493..98f90ff 2019-03-26 02:06:15 +0100 to 2019-04-06 09:29:08 -0700 - Document repr packed(N). (rust-lang/reference#553) - Fix broken link in glossary. (rust-lang/reference#558) - Typo fixes (rust-lang/reference#556) ## embedded-book 2 commits in 07fd3880ea0874d82b1d9ed30ad3427ec98b4e8a..7989c723607ef5b13b57208022259e6c771e11d0 2019-03-27 15:40:52 +0000 to 2019-04-04 12:14:37 +0000 - fix rust-embedded/book#182 (rust-embedded/book#183) - Add openocd to list of installable packages (rust-embedded/book#179)

Document repr packed(N).

f5c2b0b

Havvy approved these changes Mar 25, 2019

View reviewed changes

Centril reviewed Mar 25, 2019

View reviewed changes

Centril and others added 2 commits March 25, 2019 10:56

Apply suggestions from Centril

915ef66

Co-Authored-By: ehuss <eric@huss.org>

Try a different way to document align/packed.

a5852dd

Centril reviewed Mar 26, 2019

View reviewed changes

Centril and others added 2 commits March 26, 2019 10:49

Apply suggestions from Centril

597d727

Co-Authored-By: ehuss <eric@huss.org>

Attempt to remove duplication between align and packed.

38b408a

Centril approved these changes Mar 27, 2019

View reviewed changes

ehuss merged commit 98f90ff into rust-lang:master Apr 6, 2019

ehuss mentioned this pull request Apr 6, 2019

Update books rust-lang/rust#59754

Merged

ehuss mentioned this pull request Apr 21, 2019

Clarify packed/aligned reprs. #564

Open

Conversation

ehuss commented Mar 25, 2019

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

Havvy Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

ehuss commented Mar 25, 2019

Uh oh!

Uh oh!

Centril Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Centril Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Centril Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehuss commented Mar 26, 2019

Uh oh!

Centril commented Mar 27, 2019

Uh oh!

retep998 commented Apr 6, 2019

Uh oh!

Centril commented Apr 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants