Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

retep998
Copy link
Contributor

@retep998 retep998 commented Dec 7, 2015

Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/otherwise//

Signed-off-by: Peter Atashian <retep998@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this paragraph.

Signed-off-by: Peter Atashian <retep998@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be resolved. Verify against the behavior of clang (or gcc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically clang or gcc on a platform other than Windows. On Windows they are supposed to match the msvc ABI.

@emberian
Copy link
Contributor

emberian commented Dec 7, 2015

The feature is necessary. If this behavior is in line with common C compilers, :shipit:

Copy link
Contributor

Choose a reason for hiding this comment

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

s/effectivally/effectively/

@DanielKeep
Copy link

For reference, here's an example of Clang doing this.

Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
@retep998
Copy link
Contributor Author

retep998 commented Dec 7, 2015

See this comment for an interesting interaction edge case with the alignment RFC regarding required alignment.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Dec 8, 2015
@nikomatsakis nikomatsakis self-assigned this Dec 17, 2015
@nikomatsakis
Copy link
Contributor

Hear ye, hear ye! This RFC is now entering final comment period.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 8, 2016
@strega-nil
Copy link

This seems right to have. I think the best is #[repr(packed(1))] or #[repr(packed = "1")]. So the default of #[repr(packed)] would be #[repr(packed(1))].

@solson
Copy link
Contributor

solson commented Apr 11, 2016

@ubsan #[repr(packed(1))] is not currently valid attribute syntax (but #[repr(packed = "1")] is). I guess the RFC mentions this, so cc @retep998 as well.

It could become valid with #1559, though, or as @nrc mentions in a comment, the procedural macros RFC.

@retep998
Copy link
Contributor Author

I don't mind the syntax either way, as long as I can have this attribute at all.

@VoidStarKat
Copy link

I think it should just extend #[repr(packed)] rather than adding a new pack syntax. The duplication is confusing otherwise. Just have #[repr(packed)] be default to #[repr(packed = "1")].

@nikomatsakis
Copy link
Contributor

Hmm, I think I too prefer #[repr(packed = "N")]. Otherwise, the RFC seems unobjectionable to me. It'd also be nice to approve #1358, which covers very similar subject matter.

@nikomatsakis
Copy link
Contributor

One area the RFC did not (explicitly) address -- or I missed it -- is what to do if there are multiple packed specifiers. It did say that #[repr(pack = "2")] and #[repr(packed)] cannot be combined, but it did not say anything about #[repr(pack = "2")] and #[repr(pack = "4")]. Presumably this is an error as well? (Unlike the proposed align RFC?)

Is there any reason one might want multiple specifiers? Like, independent macros might add tighter and tighter requirements? (However, unlike with alignment, it's not obvious to me whether you would want the max or min packing that was specified.)

@retep998
Copy link
Contributor Author

I think if we did allow specifying the packing multiple times, that the right thing to do would be to use the smallest packing specified. The alignment of any given field is min(default, packing) so extending packing to be min(packing1, packing2) seems fairly natural.

As for the align RFC, there'd need to be a decision on how it interacts with packing. Whether it does the msvc style __declspec(align(N)) with required alignment that cannot be lowered by packing, or the gcc style alignment attribute that can be lowered by packing.

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC, with the caveat that the attribute be renamed to packed (and not pack). I will make this change when merging (I assume @retep998 you do not object.)

We discussed a few other things:

  • We should (obviously) clarify the relationship to an align attribute. We're going to bring the align RFC into FCP, discussion can happen there. But in the @rust-lang/lang meeting, I think the general feeling was that if both attributes are present, then the specified align would overrule the alignment derived from packing (in other words, we would match the MSVC style). Another option is to forbid both options from being used simultaneously, but that seems unnecessary to me.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#33158

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

nikomatsakis added a commit to nikomatsakis/rfcs that referenced this pull request Apr 22, 2016
also adjust from `#[repr(pack = "N")]` to `#[repr(packed = "N")]`
@nikomatsakis nikomatsakis merged commit cbb18a4 into rust-lang:master Apr 22, 2016
@Centril Centril added A-repr #[repr(...)] related proposals & ideas A-machine Proposals relating to Rust's abstract machine. A-data-types RFCs about data-types A-packed Proposals relating to types with a packed representation. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-data-types RFCs about data-types A-machine Proposals relating to Rust's abstract machine. A-packed Proposals relating to types with a packed representation. A-repr #[repr(...)] related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.