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

Update documentation for arbitrary_enum_discriminant feature #639

Open
wants to merge 2 commits into
base: master
from

Conversation

@jswrenn
Copy link

@jswrenn jswrenn commented Jul 14, 2019

Updates documentation to reflect the still unstable arbitrary_enum_discriminantfeature (tracking issue: rust-lang/rust#60553).

Copy link
Collaborator

@Havvy Havvy left a comment

Lots of good work. ❤️

Lots of little changes requested; a couple bigger ones.

<ol>
<li>

if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

Suggested change
if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:
if the enumeration is field-less. For example:

This comment has been minimized.

@jswrenn

jswrenn Jul 15, 2019
Author

Per #639 (comment), these terms may not be truly interchangeable.

if the enumeration is "C-like" (i.e., it has no tuple or struct variants); e.g.:

```rust
# #![feature(arbitrary_enum_discriminant)]

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

Feature flags aren't needed; the prose will only be merged after the feature is stable.

Suggested change
# #![feature(arbitrary_enum_discriminant)]
src/items/enumerations.md Outdated Show resolved Hide resolved

#### Casting

If there is no data attached to *any* of the variants of an enumeration, then

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

There's no need to describe a field-less enum here. Just use the term.

Suggested change
If there is no data attached to *any* of the variants of an enumeration, then
If the enum is a field-less enum, then

This comment has been minimized.

@jswrenn

jswrenn Jul 15, 2019
Author

Per #639 (comment), these terms may not be completely interchangeable. If "field-less" is meant to be synonymous to "C-like", then this change would be incorrect.

If there is no data attached to *any* of the variants of an enumeration,
then the discriminant can be directly chosen and accessed.
Each enum instance has a _discriminant_: an integer logically associated to it
that is used to determine which variant it holds. An opaque reference to this

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

Using mem::discriminant should be in the accessing the discriminant section.

integer associated to it that is used to determine which variant it holds. An
opaque reference to this discriminant can be obtained with the
[`mem::discriminant`] function.
called an enum variant.

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

The re-organization looks fine, but we lose our definition of a field-less enum. This suggestion re-adds it. I don't actually know if the anchor actually works. It should just create an anchor that can be linked too without changing any styles or making it clickable.

Suggested change
called an enum variant.
called an enum variant.
An enum where no constructors contain fields are called a *<a name="field-less-enum">field-less enum</a>*.
if a [primitive representation] is used; e.g.:

```rust
# #![feature(arbitrary_enum_discriminant)]

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

Feature flag not needed.

Suggested change
# #![feature(arbitrary_enum_discriminant)]
src/items/enumerations.md Outdated Show resolved Hide resolved
an `isize` value. However, the compiler is allowed to use a smaller type (or
another means of distinguishing variants) in its actual memory layout.

If the [primitive representation] or the [`C` representation] is used, the

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

I don't understand what this paragraph is trying to say.

leading bytes of a variant (e.g., two bytes if `#[repr(u16)]` is used), will
correspond exactly to the discriminant.

### Assigning Discriminant Values

This comment has been minimized.

@Havvy

Havvy Jul 15, 2019
Collaborator

For this entire section, it only makes sense for field-less enums or enums with a primitive representation (or the C representation?). The way it is written, it seems like explicit discriminants are only for those and implicit applies to all. Instead, it should state that having a non-opaque (feel free to pick a different descriptor) discriminant applies in those situations and in all others, it is opaque and must only be accessed via mem::discriminant.

Co-Authored-By: Ryan Scheel <Ryan.havvy@gmail.com>
@jswrenn
Copy link
Author

@jswrenn jswrenn commented Jul 15, 2019

Thank you so much for your review, @Havvy! If it's alright, I'm going to defer removing the feature flags until this gets a little closer to being merged; otherwise, I can't run mdbook tests.

Terminology

I have a lot of uncertainty about terminology. I only just saw that there's prior agreement (rust-lang/rust#46348, #244) to move away from the term "C-like enumeration" (which I believe is a very poor term) and towards "field-less enumeration". I am thrilled to strike the phrase "C-like" from this PR.

However, using "field-less enumeration" as a synonym for "C-like" isn't quite appropriate.

A C-like enumeration is one in which every variant is unit-like; for example:

enum CLike {
  VariantA,
  VariantB,
  VariantC,
}

All C-like enumerations are field-less, because they cannot include tuple-like or struct-like variants. However, not all field-less enums are are C-like. For instance:

enum Fieldless {
    Unit,
    Tuple(),
    Struct{},
}

This distinction unfortunately (ugh) matters:

  1. Only C-like enums may specify explicit discriminant without specifying a repr.
  2. Only field-less enums (regardless of whether they're also C-like) may be as-casted to their discriminant values.

It would be nice to have terms to describe both of these sets. My opinion:

  • "Field-less" is the right term to describe the set of enums which can be as-casted
  • However it cannot then also be used as a synonym for "C-like", so we should identify an alternative term to "C-like" to describe enums in which all variant are unit-like.
@Havvy
Copy link
Collaborator

@Havvy Havvy commented Jul 16, 2019

Well, that is quite unfortunate of an edge case. I'm failing to think of any good names.

@ehuss
Copy link
Collaborator

@ehuss ehuss commented Jul 16, 2019

@jswrenn Just letting you know that the reference has migrated to mdbook 0.3 (from 0.1). This means that the style of links are slightly different. They should use the .md extension, and are relative to the page they are on (previously they were relative to the root of the book). It looks like this PR should be fairly easy to rebase to resolve the conflicts. Please don't hesitate to ask if you have any questions or any trouble rebasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.