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

Allow type aliases in enumeration repr attributes. #1605

Closed
wants to merge 1 commit into from
Closed

Allow type aliases in enumeration repr attributes. #1605

wants to merge 1 commit into from

Conversation

fiveop
Copy link

@fiveop fiveop commented May 2, 2016

Allows type aliases of rust integer types to be used in #[repr(...)] attributes of C-like enumerations to specify variant representation.

Rendered

@alilleybrinker
Copy link

Are there any potential issues with public type aliases from another module being used in an attribute? I'm not aware of anywhere else that name lookup of that sort occurs in an attribute context (although I could be horribly off base on this).

@fiveop
Copy link
Author

fiveop commented May 2, 2016

@eddyb mentioned that this is currently not possible, because of syntax restrictions. However, if I understood him correctly, this problem will resolve itself due to some other changes to the language. I do not know whether I should have mentioned that in the RFC. My naive reasoning was: if the rfc is accepted, then necessary changes to the syntax will get implemented along the way.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 4, 2016
@raphaelcohn
Copy link

Just to put some meat on the bones, so to speak, here's an example piece of code where this would help. I've been writing rust for about 5 days, and, the first time I used #[repr(...)], this bit me - mostly because I just did what seemed logical...

extern crate libc;
use self::libc::c_int;

#[repr(c_int)]
pub enum Facility
{
    LOG_KERN = self::libc::LOG_KERN,
    // ... Omitted ...
}

(Note, LOG_KERN is not yet defined in libc, but I'm working on a PR for it). The type alias for LOG_KERN is c_int, which resolves to i32 on Mac OS X.

For me, this change is important because:-

  1. It clearly communicates meaning;
  2. It reduces the chance of long term breakage (and one that might not be so easily spotted, as it'll only come to light with certain crate combinations with libc, say)
  3. It does "what's right" - it matches logical intuition, rather than requiring a detailed search of the docs.

@eddyb
Copy link
Member

eddyb commented May 5, 2016

Wouldn't this make #[repr(C)], #[repr(packed)] and others ambiguous?
EDIT: Already addressed.

@fiveop
Copy link
Author

fiveop commented May 5, 2016

The current state of the RFC proposes #[repr(type = c_int)] for @raphaelcohn's example. You already expressed this concern in IRC before I wrote the PR. The ambiguity is mentioned in the Alternatives section.

As another example, as @raphaelcohn provided one, I refer to my PR nix-rust/nix#362.

@solson
Copy link
Member

solson commented May 9, 2016

Sorry if it's already been addressed, but I didn't see anyone mention this when I skimmed:

The syntax #[repr(type = c_int)] does not currently parse in rustc. To work within the existing syntax it would have to be #[repr(type(c_int))] (weird), #[repr(c_int)] (ambiguous), or #[repr(type = "c_int")] (with some precedent in the form of #[cfg(target_pointer_width = "64")], etc.).

Alternately, this RFC could lend more weight to @nrc's plans to make attribute syntax more free-form.

@nikomatsakis
Copy link
Contributor

So, this basically seems harmless to me, but there are some implementation complexities to overcome. For example, we'd want this to integrate well with hygiene.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 22, 2016

Hear ye, hear ye! This RFC is now entering final comment period with an inclination to close. We discussed in the @rust-lang/lang meeting and decided that while the RFC is well-motivated, it doesn't sufficiently address the various implementation complexities that must be overcome nor the interaction with hygiene. It would make sense to extend the attribute system to support more general paths before considering this RFC (but that is a non-trivial undertaking).

@rust-lang/lang members, please check off your name to signal agreement. Leave a comment with concerns or objections. Others, please leave comments. Thanks!

@nikomatsakis nikomatsakis added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed I-nominated labels Aug 22, 2016
@pnkfelix pnkfelix mentioned this pull request Sep 1, 2016
@nikomatsakis
Copy link
Contributor

Based on the reasoning in the previous comment, we're going to close this PR:

While the RFC is well-motivated, it doesn't sufficiently address the various implementation complexities that must be overcome nor the interaction with hygiene.

Thanks @fiveop for the proposal.

clarfonthey added a commit to clarfonthey/rust-rfcs that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. 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.

8 participants