Skip to content

Conversation

@kylo5aby
Copy link
Contributor

No description provided.

Comment on lines 128 to 141
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,
);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

metaFiledsPtrIndex,
module.i32.add(
meta,
module.i32.const(MetaDataOffset.FILEDS_PTR_OFFSET),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metaFiledsPtrIndex,
metaFieldsPtrIndex,

}

export const enum MetaFieldOffset {
export const SIZE_OF_META_FIELD = 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Contributor Author

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.

CLOSURE_CALL,
CONSTRUCTOR_CALL,
ANY_CALL,
WASM_FUNCTION_CALL,
Copy link
Contributor

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.

/** 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 {
Copy link
Contributor

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.

Comment on lines 159 to 161
/** it uses to specified a wasm function call in compile time,
* sometimes the wasm function maybe is not generated from a ts function
*/
Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

Copy link
Contributor

@xujuntwt95329 xujuntwt95329 left a comment

Choose a reason for hiding this comment

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

LGTM

@xujuntwt95329 xujuntwt95329 merged commit be2cb6f into web-devkits:main Oct 8, 2023
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.

2 participants