[RFC 2195] Document new type representations #246

Havvy · 2018-02-17T20:19:43Z

This PR does two things:

Documents RFC 1240.
Splits the Enum sections for the C and Primitive representations into field-less enums and enums with fields.

Incidentally, fixes #244

Havvy · 2018-02-18T09:26:38Z

As this is Gankro's RFC,

r? @gankro

alercah · 2018-02-18T03:25:36Z

src/type-layout.md

+
+> Note: This is commonly different than what is done in C and C++. Projects in
+> those languages often use a tuple of `(enum, payload)`. For making your enum
+> represented like that, see [the tagged union representation] below.


Tagged union representation is a broken link? Looks like you missed it.

alercah · 2018-02-18T03:27:59Z

src/type-layout.md

-It is an error for [zero-variant enumerations] to have the `C` representation.
+For enums with fields, the `C` representation has the same representation as
+it would with the [primitive representation] with the field-less enum in its
+description having the `C` representation.


This doesn't read correctly to me, I think. The primitve representation reference is circular with the section below, and there's no explicit mention in the text here of the (tag, value) pair representation.

alercah · 2018-02-19T15:55:48Z

src/type-layout.md

+`C` of two fields where the first field is a field-less enum with the `C`
+representation that has one variant for each variant in the enum with fields
+and the second field a union with the `C` representation that's fields consist
+of structs with the `C` representation corresponding to each variant in the


This is a very long sentence that's hard to parse. I'd recommend splitting the description of the fields into separate sentences, and perhaps rather than writing "with the C representation" inline in each of them, just write at the end something like "The structs and unions declared here all have the C representation themselves."

Yeah, I wasn't happy with it either. I had a personal breakthrough in how I can describe it, so now it's longer and with paragraph breaks. Saying they all have C representation on its own line is also a nice win. 👍 I'm leaving for awhile, and I need to do the same to the primitive repr, so don't look for the change yet.

alercah · 2018-02-19T15:56:09Z

src/type-layout.md


-It is an error for [zero-variant enumerations] to have a primitive
-representation.
+#### Primitive Fepresentation of Field-less Enums


typo: Fepresentation

alercah · 2018-02-19T15:56:53Z

src/type-layout.md

+the same field-less enum with the same primitive representation that is
+the enum with all fields in its variants removed and the rest of the fields
+consisting of the fields of the corresponding variant in the order defined in
+original enumeration.


This sentence is not as bad, but it might be possible to clean it up somewhat as well.

alercah · 2018-02-19T15:57:02Z

src/type-layout.md

+
+> Note: This is commonly different than what is done in C and C++. Projects in
+> those languages often use a tuple of `(enum, payload)`. For making your enum
+> represented like that, use the `C` representation.


Perhaps link up to C representation here?

alercah · 2018-02-19T15:57:57Z

src/type-layout.md

+consisting of the fields of the corresponding variant in the order defined in
+original enumeration.
+
+Because unions with non-copy fields aren't allowed, this representation can only


This doesn't appear to be the case: https://play.rust-lang.org/?gist=6d0c155ebdffe17c6a3cd9683318209b&version=nightly

alercah · 2018-02-19T16:00:51Z

src/type-layout.md

+
+// has the same type layout as this union
+#[repr(C)]
+#[derive(Clone, Copy)]


I'd omit the derive here; it doesn't tell us anything about the layout and isn't always correct.

Gankra

Good start, heavy edits needed (go to bed before 1AM next time 😉)

Some issues may be philosophical ones with existing patterns in this document.

Gankra · 2018-02-19T23:48:05Z

src/type-layout.md

@@ -149,7 +149,8 @@ layout such as reinterpreting values as a different type.
 Because of this dual purpose, it is possible to create types that are not useful
 for interfacing with the C programming language.

-This representation can be applied to structs, unions, and enums.
+This representation can be applied to structs, unions, and enums. The exception
+is [zero-variant enumerations] for which the `C` representation is an error.


You seem to use "enums" in subsequent sections, but "enumerations" here? (I see this reference name is pre-existing but metadata shouldn't affect the actual text)

Gankra · 2018-02-19T23:50:38Z

src/type-layout.md


-For all other enumerations, the layout is unspecified.
+For enums with fields, the `C` representation is defined to be the same as the
+follow types. These types don't actually exist, so the names are only here to


I don't understand what "follow types" means.

Gankra · 2018-02-20T00:26:15Z

src/type-layout.md

+The enums with fields with the `C` representation, the represented enum, has
+the same representation of a a struct two fields, the tagged union. The first
+field of the tagged union is a field-less enum, the discriminant enum. The
+second field of the tagged union is a union, the fields union.


This paragraph is really hard to understand, even as someone who knows exactly what it's describing. I think we need to establish some more standard terms/short-hands for talking about type layouts. For example, "type with the C representation" could just be "repr(C) type".

It's also important here that it is not just the layout as the term is defined by this document, but also the ABI that matches. This is important for e.g. passing this type by-value in a C FFI function. I think we should either define layout to include ABI, or create a term for "layout and ABI". Here I'm going to use "representation" to refer to layout+ABI, and also use it to refer the "desugared" type to provide a potential rewrite (with an incredibly long pedantic aside):

The representation of a repr(C) enum with fields is a repr(C) struct with two fields:

a repr(C) version of the enum with all fields removed ("the tag")

a repr(C) union of repr(C) structs for the fields of each variant that had them ("the payload")

(Note: due to the representation of repr(C) structs and unions, if a variant has a single field there is no difference between putting that field directly in the union or wrapping it in a struct; any system which wishes to manipulate such an enum's representation may therefore use whichever form is more convient/consistent for them)

Gankra · 2018-02-20T00:31:03Z

src/type-layout.md

+struct FieldsC { x: u32, y: u8 }
+
+#[repr(C)]
+struct FieldsD;


Note: you need to derive(Copy, Clone) on all the field structs for this example to compile on today's Rust because union's can't contain non-copyable types, although it doesn't affect the layout/repr so it might be reasonable to omit it in this document? (do we need to mark this as ignore if we do that?)

Also note: FieldsD could be omitted, and it must be in C(++) headers, so I am slightly inclined to simply omit it. Worth discussing this footgun? Or is that more of a nomicon thing.

Gankra · 2018-02-20T00:38:00Z

src/type-layout.md

+```rust
+// This Enum has the same layout as
+#[repr(C)]
+enum RepresentedEnum {


I think this example would benefit from some tweaked type names to better emphasize the connection

enum MyEnum struct MyEnumRepr enum MyEnumDiscriminant (would honestly prefer tag if you'll allow it, but I understand discriminant might be more consistent) union MyEnumFields struct MyEnumAFields (maybe remove MyEnum from this one?)

Gankra · 2018-02-20T15:50:15Z

src/type-layout.md

+
+For [field-less enums], they set the size and alignment to be the same as
+the primitive type of the same name. For example, a field-less enum with
+a `u8` representation can only have discriminants between 0 and 255 inclusive.


I believe it also gives the type the same ABI as a primitive int (e.g. it would be passed in a register instead of on the stack on some x86 ABIs)

Gankra · 2018-02-20T16:26:10Z

src/type-layout.md


-For all other enumerations, the layout is unspecified.
+Because unions with non-copy fields aren't allowed, this representation can only
+be used if every field is also [`Copy`].


"can only be used" -> "can only be expressed in Rust", maybe? (C(++) can use it fine, and you could do some really hacky crap to use it in Rust too)

Gankra · 2018-02-20T16:38:20Z

src/type-layout.md

+the same field-less enum with the same primitive representation that is
+the enum with all fields in its variants removed and the rest of the fields
+consisting of the fields of the corresponding variant in the order defined in
+original enumeration.


This one is definitely better than the repr(C) version, but I think we can also improve it. I suggest a similar rewrite (with another too long pedantic point I just want to write down somewhere:

The representation of a repr(int) enum is a repr(C) union of repr(C) structs for each variant with a field. The first field of each struct in the union is a repr(int) version of the enum with all fields removed ("the tag") and the remaining fields are the fields of that variant.

Note: this representation is unchanged if the tag is given its own member in the union, should that make manipulation more clear for you (although in C++, to follow The Exact Word Of The Standard the tag member should be wrapped in a struct).

Gankra · 2018-02-20T17:15:32Z

src/type-layout.md

-Likewise, combining two primitive representations together is unspecified.
+> Note: This is commonly different than what is done in C and C++. Projects in
+> those languages often use a tuple of `(enum, payload)`. For making your enum
+> represented like that, use the `C` representation.


I'm not sure this is necessary. Or at least I would eliminate the reference to "what's commonly done in C(++)" which is like, never a true statement.

Gankra · 2018-02-20T17:16:35Z

src/type-layout.md

+> represented like that, use the `C` representation.
+
+```rust
+// This custom enum


Similar notes on this example as the previous (although you used my preferred naming scheme on this one..?)

The last commit was a WIP where I was transitioning from this style (that you like better) to the style I used in #[repr(C)] (that you like less).

alercah · 2018-08-03T13:49:03Z

@Havvy what's the status of this?

Havvy · 2018-08-03T19:18:25Z

The somewhat conflicting feedback scared me on this, and I still need to actually look at the feedback...

Centril · 2019-01-10T18:57:01Z

Ping @Havvy, did you get a chance to actually look at the feedback?

Havvy · 2019-07-13T07:43:29Z

I don't know if and when I'd get back to this, and it needs a lot of work. So I'm going to just close this.

Document new type representations

ad4342b

Havvy changed the title ~~[Document new type representations~~ [WIP] Document new type representations Feb 17, 2018

Organization of reprs

e8aabe8

Havvy force-pushed the type-layout branch from c813590 to e8aabe8 Compare February 18, 2018 09:25

Havvy changed the title ~~[WIP] Document new type representations~~ Document new type representations Feb 18, 2018

Havvy changed the title ~~Document new type representations~~ [RFC 2195] Document new type representations Feb 19, 2018

alercah reviewed Feb 19, 2018

View reviewed changes

WIP Address alercah's concerns

c883ab2

Gankra requested changes Feb 20, 2018

View reviewed changes

Havvy closed this Jul 13, 2019

ehuss mentioned this pull request Apr 4, 2020

enum documentation not updated for RFC 2195 #786

Closed

poliorcetics mentioned this pull request Aug 21, 2020

Document the enum changes in RFC 2195 #879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC 2195] Document new type representations #246

[RFC 2195] Document new type representations #246

Havvy commented Feb 17, 2018 •

edited

Loading

Havvy commented Feb 18, 2018

alercah Feb 18, 2018

alercah Feb 18, 2018

alercah Feb 19, 2018

Havvy Feb 19, 2018 •

edited

Loading

alercah Feb 19, 2018

alercah Feb 19, 2018

alercah Feb 19, 2018

alercah Feb 19, 2018

alercah Feb 19, 2018

Gankra left a comment

Gankra Feb 19, 2018

Gankra Feb 19, 2018

Havvy Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Gankra Feb 20, 2018

Havvy Feb 20, 2018

alercah commented Aug 3, 2018

Havvy commented Aug 3, 2018

Centril commented Jan 10, 2019

Havvy commented Jul 13, 2019

[RFC 2195] Document new type representations #246

[RFC 2195] Document new type representations #246

Conversation

Havvy commented Feb 17, 2018 • edited Loading

Havvy commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Havvy Feb 19, 2018 • edited Loading

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

Gankra 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

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

alercah commented Aug 3, 2018

Havvy commented Aug 3, 2018

Centril commented Jan 10, 2019

Havvy commented Jul 13, 2019

Havvy commented Feb 17, 2018 •

edited

Loading

Havvy Feb 19, 2018 •

edited

Loading