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 enum discriminants to not be uint, and use smallest possible size by default. #9006

Closed
wants to merge 6 commits into from

Conversation

jld
Copy link
Contributor

@jld jld commented Sep 6, 2013

This is the long-awaited branch to allow enum discriminants to be any integral type. The default is the smallest one that can represent the possible values, but this can be overridden by an attribute, either to a specific type or to whatever the platform's C ABI would use. The ctypes lint pass is extended to detect non-FFI-safe enums in foreign item types.

I've fixed the months of bit rot, and it passes make check on x86_64 Linux, and I've squashed it into a reasonably comprehensible form — but it definitely needs more tests, and I think some of the commit messages could stand to be more informative. The former could be addressed in a followup PR, to reduce the possibility of further bit rot.

Also export enum attrs into metadata, and add a convenient interface for
obtaining the repr hint from either a local or remote definition.
Note that raising an error during trans doesn't stop the compile or cause
rustc to exit with a failure status, currently, so this is of more than
cosmetic importance.
The variant used in debug-info/method-on-enum.rs had its layout changed
by the smaller discriminant, so that the `u32` no longer overlaps both
of the `u16`s, and thus the debugger is printing partially uninitialized
data when it prints the wrong variant.

Thus, the test runner is modified to accept wildcards (using a string
that should be unlikely to occur literally), to allow for this.
@jld
Copy link
Contributor Author

jld commented Sep 6, 2013

f?(@brson, @nikomatsakis, …); feel free to tag whoever else looks after these parts of the code these days.

@jld
Copy link
Contributor Author

jld commented Sep 6, 2013

@bors: retry

@huonw
Copy link
Member

huonw commented Sep 6, 2013

(cc @kemurphy)

let discriminant_type_metadata = |inttype| {
let discriminant_llvm_type = adt::ll_inttype(cx, inttype);
let (discriminant_size, discriminant_align) = size_and_align_of(cx, discriminant_llvm_type);
let discriminant_type_metadata = type_metadata(cx, match inttype {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating the logic here, I'd rather use adt::ty_of_inttype() so it doesn't go out of sync.

@michaelwoerister
Copy link
Member

This looks great :)
Making the debuginfo test runner more flexible also seems like a sensible idea to me.

match item.node {
ast::MetaWord(word) => {
let word: &str = word;
let hint = match word {
Copy link
Member

Choose a reason for hiding this comment

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

This could be match word.as_slice() { rather than using the temporary &str manually. (Just a note; it doesn't matter particularly.)

@huonw
Copy link
Member

huonw commented Sep 6, 2013

Does this happen to have any bearing on #8761?

@thestinger
Copy link
Contributor

@jld: there's a real failure in the second merge/test attempt

@brendanzab
Copy link
Member

@jld Is it possible to use type aliases with this? Like, #[repr(c_int)]?

@catamorphism
Copy link
Contributor

Closing due to lack of activity -- please reopen if necessary!

@jld
Copy link
Contributor Author

jld commented Sep 25, 2013

Yes, that definitely is an error in the second test run, and I'm going to have to alter the visitor interface to fix it....

@jld
Copy link
Contributor Author

jld commented Sep 30, 2013

Superseded by #9613. (Making a new PR so I can change the branch name to not say “for feedback”.)

Answering lingering questions:

  • No, type aliases like std::core::libc::c_int can't be used; things would have to call into name resolution for that, I think. But it might be useful to have.
  • No, no effect on C-like enums discriminants values can be given different types #8761. It might be possible to specify discriminant type that way, but I haven't tried implementing it. One related thing I did try was to constrain the discriminant constants' types if a #[repr] was present, to catch out-of-range values, but that doesn't work — I think it just silently truncates them instead.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2022
feat(fix): ignore `todo!` and `unimplemented!` in `if_same_then_else`

close: rust-lang#8836
take over:  rust-lang#8853

This PR adds  check `todo!` and `unimplemented!` in if_same_then_else.
( I thought `unimplemented` should not be checked as well as todo!.)

Thank you in advance.

changelog: ignore todo! and unimplemented! in if_same_then_else

r? `@Jarcho`
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

9 participants