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

Documented the #[repr(align(x))] attribute. #182

Merged
merged 1 commit into from
Jan 25, 2018

Conversation

bitshifter
Copy link
Contributor

Updated reference for stabilization of the #[repr(align(x))] attribute. See tracking issue comments rust-lang/rust#33626 (comment) for discussion.

@Havvy
Copy link
Contributor

Havvy commented Dec 26, 2017

I thought I removed most of the verbiage of the repr attribute from the attributes page. Whoops.

You'll instead want to add a section ```The align representation` to `type-layout.md`.

@bitshifter
Copy link
Contributor Author

Ah OK, should I leave the text I added to attributes.md since it's already talking about the other repr attributes?

@Havvy
Copy link
Contributor

Havvy commented Dec 26, 2017

It's better to just link to the full description of the representation attribute from the attributes.md page, so as to avoid duplicating information too much. I just forgot to clean up the attributes.md page when the type layout page landed.

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

The prose is good, just the paragraph structure feels off.

Also, since Github doesn't let me put a comment on lines you're not touching, on line 109 of type layout, you need to add align to the list.

Also, Github doesn't notify me when there are new commits. Leave a comment or else I won't see it quickly.

@@ -264,13 +264,23 @@ For all other enumerations, the layout is unspecified.

Likewise, combining two primitive representations together is unspecified.

### The `packed` Representation
### `align(x)` Representation
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be called The `align` Representation.

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

Github ate most of my previous review comments. ;.; So here's a redo at the review.

two of type `u32`. The `align` representation cannot lower the alignment of a
type.

### `packed` Representation
Copy link
Contributor

Choose a reason for hiding this comment

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

You got rid of the The here.


The `packed` representation can only be used on `struct`s and `union`s.

It modifies the representation (either the default or `C`) by removing any
padding bytes and forcing the alignment of the type to `1`.

The `align` and `packed` representations cannot be applied on the same type and
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should also be included in the align representation section.

### The `packed` Representation
### `align(x)` Representation

The `align(x)` representation can be used on `struct`s and `union`s to raise the
Copy link
Contributor

Choose a reason for hiding this comment

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

The paragraph structure here should mirror the packed representation section. So which categories of types it can be applied to should be one paragraph, and what it does should be another.

### The `packed` Representation
### `align(x)` Representation

The `align(x)` representation can be used on `struct`s and `union`s to raise the
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the (x) from the name. Instead, describe how it takes the alignment as a parameter in the paragraph describing how it works.

@bitshifter
Copy link
Contributor Author

Thanks @Havvy I think I've addressed your feedback. This stuff hasn't been stabilized quite yet so you might want to wait until that happens before accepting the PR I guess.

@Havvy
Copy link
Contributor

Havvy commented Dec 30, 2017

Github shows that you did a force push, but I don't actually see any changes in the new commit. I even checked your branch and there's no difference there either.

@bitshifter
Copy link
Contributor Author

Whoops, I did a git commit --amend but forgot to add first. Should be updated now.

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

Waiting on stabilization in the compiler.

@Havvy
Copy link
Contributor

Havvy commented Dec 31, 2017

I have not updated the reference Attribute syntax grammar documentation as the new syntax is only used by #[repr(align)] and I thought it might be pretty confusing to include a grammar which is only used by one kind of attribute for now. &mdash @bitshifter; rust-lang/rust #33626

Even if it's a change used by one attribute, it should still be in the attribute grammar. Do you want to add it? If not, I'll do so in a separate PR.

@bitshifter
Copy link
Contributor Author

Sorry, I didn't see your comment about the grammar. I can look at adding it.

@bitshifter
Copy link
Contributor Author

Updated the grammar, hopefully I got it right! It looks like my stabilization PR is almost in.

@Havvy
Copy link
Contributor

Havvy commented Jan 25, 2018

@bitshifter There's a merge conflict. 😢

@bitshifter
Copy link
Contributor Author

@Havvy the conflict was on a section on repr attributes that I removed as part of this review, had recently been updated by @yellow-apple. Thus I've removed the edit with the rest of the section, but perhaps @yellow-apple should check the type-layout.md section on #[repr(C)] Enums to make sure it contains the same information (it's worded differently but seems semantically similar to me).

@Havvy Havvy merged commit fc4fda4 into rust-lang:master Jan 25, 2018
@Havvy
Copy link
Contributor

Havvy commented Jan 25, 2018

💟 Thanks!

@bitshifter bitshifter deleted the stabilize-repr-align branch January 28, 2018 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants