-
Notifications
You must be signed in to change notification settings - Fork 26
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
support for in loop for class and interface #15
Conversation
Signed-off-by: zhenweijin <[email protected]>
Signed-off-by: zhenweijin <[email protected]>
const vtable = binaryenCAPI._BinaryenStructGet( | ||
module.ptr, | ||
StructFieldIndex.VTABLE_INDEX, | ||
obj, | ||
binaryen.i32, | ||
false, | ||
); | ||
const metaValue = binaryenCAPI._BinaryenStructGet( | ||
module.ptr, | ||
VtableFieldIndex.META_INDEX, | ||
vtable, | ||
binaryen.i32, | ||
false, | ||
); |
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.
I think "get vtable" and "get meta" should become two helper functions
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.
Good idea.
Signed-off-by: zhenweijin <[email protected]>
metaFiledsPtrIndex, | ||
module.i32.add( | ||
meta, | ||
module.i32.const(MetaDataOffset.FILEDS_PTR_OFFSET), |
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.
module.i32.const(MetaDataOffset.FILEDS_PTR_OFFSET), | |
module.i32.const(MetaDataOffset.FIELDS_PTR_OFFSET), |
// 3. get meta fields ptr | ||
statementArray.push( | ||
module.local.set( | ||
metaFiledsPtrIndex, |
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.
metaFiledsPtrIndex, | |
metaFieldsPtrIndex, |
@@ -1623,18 +1622,67 @@ export const enum VtableFieldIndex { | |||
META_INDEX = 0, | |||
} | |||
|
|||
export const enum MetaFieldOffset { | |||
export const SIZE_OF_META_FIELD = 12; |
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.
What does this mean?
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.
What does this mean?
this means the size of bytes occupied by the meta field, it helps to traverse every meta field.
src/semantics/value.ts
Outdated
@@ -52,6 +52,7 @@ export enum SemanticsValueKind { | |||
CLOSURE_CALL, | |||
CONSTRUCTOR_CALL, | |||
ANY_CALL, | |||
WASM_FUNCTION_CALL, |
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.
We can't introduce such concept in semantic tree, semantic tree should not be coupled with a specific backend.
src/expression.ts
Outdated
/** it uses to specified a wasm function call in compile time, | ||
* sometimes the wasm function maybe is not generated from a ts function | ||
*/ | ||
export class CallWASMFunction extends Expression { |
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.
Also, we shouldn't introduce such concept in frontend, this is a special design in binaryen backend.
Signed-off-by: zhenweijin <[email protected]>
src/expression.ts
Outdated
/** it uses to specified a wasm function call in compile time, | ||
* sometimes the wasm function maybe is not generated from a ts function | ||
*/ |
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.
Should update the comment
private obj: Expression; | ||
|
||
constructor(obj: Expression) { | ||
super(ts.SyntaxKind.PropertyAccessExpression); |
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.
Why we use PropertyAccessExpression
here?
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.
In ts.SyntaxKind
seems to lack an appropriate type to uniquely represent the access of enumerate keys. Therefore, here we use ts.SyntaxKind.PropertyAccessExpression
to indicate obtaining the key attribute from an object.
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.
Got it, had better leave a comment here
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.
Would it be better to use SyntaxKind.Unknown
to indicate this is a customized node?
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.
I don't think it's very suitable, because SyntaxKind.Unknown
has its own meaning in the frontend, it has already been used to represent some nodes that the compiler doesn't support or implement.
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.
OK
expr as PropertyAccessExpression, | ||
context, | ||
); | ||
if (expr instanceof EnumerateKeysExpression) { |
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.
Had better also leave a comment here
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.
Got it
Signed-off-by: zhenweijin <[email protected]>
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.
LGTM
No description provided.