-
-
Notifications
You must be signed in to change notification settings - Fork 676
feat: support element access of enum #2950
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
feat: support element access of enum #2950
Conversation
Concept ACK but I'll have to review later |
ping @CountBleck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems decent
for (let _keys = Map_keys(members), _values = Map_values(members), i = 1, k = _keys.length; i <= k; ++i) { | ||
let enumValueName = unchecked(_keys[k - i]); | ||
let member = unchecked(_values[k - i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (let _keys = Map_keys(members), _values = Map_values(members), i = 1, k = _keys.length; i <= k; ++i) { | |
let enumValueName = unchecked(_keys[k - i]); | |
let member = unchecked(_values[k - i]); | |
for (let _keys = Map_keys(members), _values = Map_values(members), i = _keys.length - 1; i >= 0; --i) { | |
let enumValueName = unchecked(_keys[i]); | |
let member = unchecked(_values[i]); |
Doesn't this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i
is signed, then it works. The original version works for unsigned and signed to avoid unexpected endless loop
return true; | ||
} | ||
|
||
private ensureEnumToString(enumElement: Enum, reportNode: Node): string | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a couple blank lines in here? It's a little hard to read
let resolver = this.resolver; | ||
let targetElement = resolver.lookupExpression(targetExpression, this.currentFlow, Type.auto, ReportMode.Swallow); | ||
if (targetElement && targetElement.kind == ElementKind.Enum) { | ||
const elementExpr = this.compileExpression(expression.elementExpression, Type.i32, Constraints.ConvImplicit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're allowing Enum[Enum.X]
but not Enum["X"]
(only Enum.X
, the property access expression, is allowed). This is likely a good thing to have, unless users really want to get enum members with computed string values (which should probably be discouraged regardless).
(It also seems like we don't have a good error for someObject["property"]
and we use TS2329 instead, so a good error for Enum["X"]
isn't necessary for now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since the current runtime only support to get enum value from compilation time known string (aka string literal). Enum.X
is always better then Enum["X"]
.
No description provided.