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

docs: small fixes for doctor-data instructions #749

Closed
wants to merge 2 commits into from
Closed

docs: small fixes for doctor-data instructions #749

wants to merge 2 commits into from

Conversation

vaeng
Copy link
Contributor

@vaeng vaeng commented Dec 2, 2023

Suggestions by @clechasseur

@vaeng vaeng self-assigned this Dec 2, 2023
Copy link
Contributor

@siebenschlaefer siebenschlaefer left a comment

Choose a reason for hiding this comment

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

If you guys really want this I'll approve.
But I like the previous version more. The "elements" of an enum are called enumerators, enum is the type itself.

@vaeng
Copy link
Contributor Author

vaeng commented Dec 3, 2023

@siebenschlaefer
Here is @clechasseur original reasoning:

In the first task, the text mentions an "enumerator of type System". I think the use of the term "enum" (or perhaps "enumeration") would be better, because "enumerator" is used in many languages to mean something quite different from a C++ enum.

I might go the middle way by inserting a short "reminder" about the naming convention of enums in C++ and reverting it to the original state. What do you think?

@vaeng vaeng mentioned this pull request Dec 3, 2023
@vaeng
Copy link
Contributor Author

vaeng commented Dec 3, 2023

Needs one more substitution: #742

@clechasseur
Copy link

Here is a more complete excerpt:

The third argument comes from a `star_map` namespace.
It is an enumerator of type `System`.
You even got one of the enumerations: `BetaHydri`.

The second sentence seems to refer to the type itself. That's why I suggested using enum instead of enumerator.

@siebenschlaefer
Copy link
Contributor

I don't object to this change. I understand that these concepts don't target language lawyers and that it might sense to use terms that are easier to understand than what is used in the standard.
I just wanted to give the C++ perspective where "enumerator" is the correct term (see [dcl.enum] in the current draft of the standard.

Copy link

@clechasseur clechasseur left a comment

Choose a reason for hiding this comment

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

@siebenschlaefer After a careful reading of your comments and the provided link, I believe that the following terms should be used

  • enumeration: refers to the type (in this case, System)
  • enumerator: refers to an enum variant (in this case, BetaHydri)

I would therefore propose the following rephrasing, which more closely matches the standard (emphasis mine):

The third argument comes from a star_map namespace.
It is an enumeration type named System.
You even got one of the enumerators: BetaHydri.

As an aside, I didn't know that the term enumerator was used in the C++ standard to refer to an enum variant. Because of that, I thought using enumerator was misleading to people coming from other languages. I do in fact like to use proper C++ terminology when possible. 🙂

@vaeng
Copy link
Contributor Author

vaeng commented Jan 12, 2024

Also add clarification to the garbled data: https://forum.exercism.org/t/corruption-in-the-c-track-repo/9088/6

[no important files changed]
@vaeng vaeng closed this Jan 22, 2024
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.

3 participants