kaitai_struct_compiler
kaitai_struct_compiler copied to clipboard
Give a more natural names to the Enum-related AST nodes
Also document that nodes and related AbstractTranslator methods.
Renames:
| Before | After |
|---|---|
Ast.expr.EnumById |
Ast.expr.EnumCast |
Ast.expr.EnumByLabel |
Ast.expr.EnumVariant |
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.
"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.
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.