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

Clarify the semantics of enum discriminants #31772

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Conversation

nodakai
Copy link
Contributor

@nodakai nodakai commented Feb 19, 2016

cf. https://doc.rust-lang.org/error-index.html#E0082

The default type for enum discriminants is isize, but it can be adjusted by adding the repr attribute to the enum declaration.

It would be great if anyone could check my English.

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@apasel422
Copy link
Contributor

@pnkfelix "specify"?

@pnkfelix
Copy link
Member

@apasel422 works for me

@Aatch
Copy link
Contributor

Aatch commented Feb 20, 2016

The error explanation is somewhat misleading, we actually use the smallest valid integer type (with a preference for unsigned integers) when no repr is supplied. While typechecking treats the RHS as an isize for, well, checking, to say it "defaults" to it implies that it will be represented as such. Especially given the context.

We probably shouldn't replicate that.

@nodakai
Copy link
Contributor Author

nodakai commented Feb 20, 2016

@Aatch Are you suggesting to fix error-index.html? Could you elaborate on what you meant by "replicate" ? I believe my patch isn't making the situation worse in terms of an ambiguity on what defaults to isize in enum definition.

@Aatch
Copy link
Contributor

Aatch commented Feb 22, 2016

@nodakai it's the same issue here as in error-index.html. It says "the default type for enum discriminants is isize", which is kinda true, but it suggests that the enum discriminants will be represented using isize by default, which isn't true. Given that both this patch and error-index.html are talking about #[repr(X)] in the same place, it actually makes the semantics less clear.

@nodakai
Copy link
Contributor Author

nodakai commented Feb 22, 2016

@Aatch So, what should I do? Are you suggesting me to fix error-index.html at the same time?

@Aatch
Copy link
Contributor

Aatch commented Feb 22, 2016

@nodakai I think I may have been misremembering your wording earlier, confusing it with the error-index.html wording, so apologies for that. Your wording is actually fine in this respect. If you want to update the error index at the same time, go for it.

@steveklabnik
Copy link
Member

I would agree with "specify"

r=me after

@nodakai
Copy link
Contributor Author

nodakai commented Mar 6, 2016

Extended the scope of the patch to include error-index.html

Avoided the word "assign" in referring to enum declarations because "assign" often implies an update of a mutable state.

@steveklabnik
Copy link
Member

@bors: r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Mar 8, 2016

📌 Commit 969d027 has been approved by steveklabnik

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Mar 8, 2016
Clarify the semantics of enum discriminants

cf. https://doc.rust-lang.org/error-index.html#E0082

> The default type for enum discriminants is isize, but it can be adjusted by adding the repr attribute to the enum declaration.

It would be great if anyone could check my English.
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Mar 8, 2016
Clarify the semantics of enum discriminants

cf. https://doc.rust-lang.org/error-index.html#E0082

> The default type for enum discriminants is isize, but it can be adjusted by adding the repr attribute to the enum declaration.

It would be great if anyone could check my English.
bors added a commit that referenced this pull request Mar 9, 2016
Rollup of 7 pull requests

- Successful merges: #31772, #32083, #32084, #32092, #32099, #32103, #32115
- Failed merges:
@bors bors merged commit 969d027 into rust-lang:master Mar 9, 2016
@nodakai nodakai deleted the patch-1 branch March 12, 2016 09:54
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.

7 participants