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

Give a more natural names to the Enum-related AST nodes #224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Nov 27, 2020

Also document that nodes and related AbstractTranslator methods.

Renames:

Before After
Ast.expr.EnumById Ast.expr.EnumCast
Ast.expr.EnumByLabel Ast.expr.EnumVariant

@GreyCat
Copy link
Member

GreyCat commented Jul 21, 2024

Not a big fan of proposed naming too. "Cast" is pretty vague (cast from what to what? to enum or from enum to something else?), "variant" is also very vague and invokes thoughts of this variant type or that variant type, neither of which seem to be closely related to getting enum object from a text label.

@Mingun
Copy link
Contributor Author

Mingun commented Jul 22, 2024

"Cast" is pretty vague

No more than static_cast, const_cast or reinterpret_cast from C++.

Compare:

// C++
enum Enum { ... }
int data;
// cast from type of `data` to `Enum`
casted = static_cast<Enum>(data);
//                   ^^^^  ~~~~
//                    to   from
// Scala
// cast from type of `data` to `Enum`
casted = Ast.expr.EnumCast(Ast.identifier("Enum"), data)
//                                         ^^^^    ~~~~
//                                          to     from

"variant" is also very vague and invokes thoughts of this variant type or that variant type, neither of which seem to be closely related to getting enum object from a text label.

This name from Rust background, where elements of enum called "variants", which, I think, very clear name.

I don't think it's worth considering the AST node type as an operation ("getting enum object from a text label"). Ast.expr.EnumVariant is a definition of enum element, where you define the type and the concrete element of this enumerated type.

The arguments of Ast.expr.EnumVariant is not expressions, but constants, so I see no reason to consider this expression as an operation rather than a constant.

@generalmimon
Copy link
Member

generalmimon commented Jul 22, 2024

To me, Id and Label were very clear names (especially "label" conveys very well that it is a text identifier). Cast and Variant not so much. The only trifle with the old names that tickles my perfectionistic brain (but it's really nothing serious) is that it should perhaps be EnumMemberById and EnumMemberByLabel (it's not really the "enum" that you resolve by ID or label, but a specific enum member), but that's more of a nitpick.

I agree with @GreyCat that Cast is not a very good name: his remark "cast from what to what?" sums up my thoughts well.

The fact that Rust uses the word "variant" for enum entries is actually somewhat confusing, because as @GreyCat pointed out, https://en.wikipedia.org/wiki/Tagged_union lists it as a possible alias of the entire "tagged union" type as a whole (and for example the C++ ecosystem seems to have adopted this, seeing boost::variant<int, std::string> in the Tagged union > 2000s section and C++17 using Tree = std::variant<struct Leaf, struct Node>; in Tagged union > 2010s). So there is some overloading of this term, and the meaning Rust uses doesn't seem that popular elsewhere.

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