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

Document the enum changes in RFC 2195 #879

Merged
merged 10 commits into from
Sep 4, 2020
Merged

Document the enum changes in RFC 2195 #879

merged 10 commits into from
Sep 4, 2020

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Aug 21, 2020

Document the changes introduced in https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md.

Fixes #244 (I believe), fixes #786.

This is your RFC so I'll tag you @Gankra.

This is a restart from #246, applying the comments that were given at the time.

Disclaimer: I have never personally used this feature and only tested using the playground so I may be missing things, I'm sorry if that's the case.

@Havvy
Copy link
Contributor

Havvy commented Aug 22, 2020

Huge thanks for taking this on! Like, have every 💟 I could ever write out.

Since you said it's based on my PR I'm going to have to bow out on reviewing it.

@Havvy Havvy requested a review from ehuss August 22, 2020 07:06
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

The writing looks good, aside from one point that I left as a review.

However, I am no expert in enum representations, so I have not checked this for factual correctness.

src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

And finally, is it possible to summarize the difference between this and "normal" primitive reps in a sentence? Something like "union-of-structs vs struct-of-tag-and-unions"?

I'm not sure I understand what it is you want

src/type-layout.md Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 26, 2020

And finally, is it possible to summarize the difference between this and "normal" primitive reps in a sentence? Something like "union-of-structs vs struct-of-tag-and-unions"?

I'm not sure I understand what it is you want

I think it would be useful to directly contrast repr(int) and repr(C, int) and explain how they differ.

src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

poliorcetics commented Aug 26, 2020

I think it would be useful to directly contrast repr(int) and repr(C, int) and explain how they differ.

I added a new subsection that explains it and shows some example. 😄

EDIT: sorry for the force push, I made a one character mistake which caused failing tests.

src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
src/type-layout.md Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Sep 1, 2020

@RalfJung Did you have any further comments on this, or does it look good to you?

@poliorcetics
Copy link
Contributor Author

Remind me to squash once all the changes are approved, I'm just waiting for all the reviews to be done. 😄

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

My only remaining comment is that this seems to use "layout" and "representation" synonymously. But this is likely pre-existing and it is something we also struggle with in the UCG. (We mostly switched to "layout" there but then realized "layout" can mean at least 3 different things... see rust-lang/unsafe-code-guidelines#122.)

My review was mostly editorial. I did not fact-check all the statements about representations and C++ here, as I am not deeply familiar with those details.

src/type-layout.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! I appreciate sticking through with all the review comments. I'll just squash via GitHub.

@ehuss ehuss merged commit 66b4d58 into rust-lang:master Sep 4, 2020
@poliorcetics poliorcetics deleted the enum-doc-2195 branch September 4, 2020 14:47
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 16, 2020
Update books

## nomicon

1 commits in 25854752549d44d76fbd7650e17cb4f167a0b8fb..6e57e64501f61873ab80cb78a07180a22751a5d6
2020-08-19 16:41:48 -0400 to 2020-09-14 11:40:23 -0400
- Fix API change to alloc::Global::grow. (rust-lang/nomicon#236)

## reference

3 commits in 25391dba46262f882fa846beefaff54a966a8fa5..56a13c082ee90736c08d6abdcd90462517b703d3
2020-09-02 07:22:55 -0700 to 2020-09-14 23:20:16 -0700
- Update the description of staticlib (rust-lang/reference#884)
- Rust 1.46 now allows more features in const fn (rust-lang/reference#883)
- Document the enum changes in RFC 2195 (rust-lang/reference#879)

## book

1 commits in e5ed97128302d5fa45dbac0e64426bc7649a558c..cb28dee95e5e50b793e6ba9291c5d1568d3ad72e
2020-08-31 12:53:40 -0500 to 2020-09-09 10:06:00 -0500
- Fixed the error message of invalid array element access in ch03.2 (rust-lang/book#2446)
ldm0 pushed a commit to ldm0/reference that referenced this pull request Sep 16, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Sep 16, 2020
Update books

## nomicon

1 commits in 25854752549d44d76fbd7650e17cb4f167a0b8fb..6e57e64501f61873ab80cb78a07180a22751a5d6
2020-08-19 16:41:48 -0400 to 2020-09-14 11:40:23 -0400
- Fix API change to alloc::Global::grow. (rust-lang/nomicon#236)

## reference

3 commits in 25391dba46262f882fa846beefaff54a966a8fa5..56a13c082ee90736c08d6abdcd90462517b703d3
2020-09-02 07:22:55 -0700 to 2020-09-14 23:20:16 -0700
- Update the description of staticlib (rust-lang/reference#884)
- Rust 1.46 now allows more features in const fn (rust-lang/reference#883)
- Document the enum changes in RFC 2195 (rust-lang/reference#879)

## book

1 commits in e5ed97128302d5fa45dbac0e64426bc7649a558c..cb28dee95e5e50b793e6ba9291c5d1568d3ad72e
2020-08-31 12:53:40 -0500 to 2020-09-09 10:06:00 -0500
- Fixed the error message of invalid array element access in ch03.2 (rust-lang/book#2446)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants