Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Conversation

@Honry
Copy link
Contributor

@Honry Honry commented Nov 4, 2019

No description provided.

@Honry Honry mentioned this pull request Nov 4, 2019
]

# Assert data
lst_oprt_with_const_assert = {}
Copy link
Member

Choose a reason for hiding this comment

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

What does the lst_oprt here 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.

It means 'list of operator'. A data list specific for "const vs const" and "param vs const" tests. I will add more annotate here.

template = Simdf32x4ArithmeticCase.gen_test_fn_template(self)

# Function template
tpl_func = ' (func (export "{}"){} (result v128) ({} {}{}))'
Copy link
Member

Choose a reason for hiding this comment

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

What does the tpl here mean? It also might be good to use names in the string interpolation so its clear what each part is supposed to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tpl means 'template'.

It also might be good to use names in the string interpolation so its clear what each part is supposed to be.

Good point! I'd like to address this in a separate PR as it is a unified improvement.

'max-abs': [
['-1.125'] * 2, ['0.125'] * 2, ['1.125'] * 2
]
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how these structures are used either. They seem different from the previous structures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These structures inherit from simd_arithmetic.py, specific for combination tests. Here should have annotate to explain this.

cases.append(str(AssertReturn(case_data[0],
[self.v128_const(case_data[3][0], case_data[1][0]),
self.v128_const(case_data[3][1], case_data[1][1])],
self.v128_const(case_data[3][2], case_data[2][0]))))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's missing a space of indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the previous data is an array, that's why it looks like unaligned.

(func (export "f64x2.max_with_const_24") (result v128) (f64x2.max (v128.const f64x2 0x00 0x01) (v128.const f64x2 0x00 0x01)))
(func (export "f64x2.max_with_const_25") (result v128) (f64x2.max (v128.const f64x2 0x02 0x80000000) (v128.const f64x2 0x02 0x80000000)))
;; f64x2.max param vs const
(func (export "f64x2.max_with_const_27")(param v128) (result v128) (f64x2.max (local.get 0) (v128.const f64x2 0 1)))
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there should be another space before the param 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.

Good catch, I will fix it.

Copy link
Contributor Author

@Honry Honry left a comment

Choose a reason for hiding this comment

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

@tlively, thanks for review, I will address the comments soon.

template = Simdf32x4ArithmeticCase.gen_test_fn_template(self)

# Function template
tpl_func = ' (func (export "{}"){} (result v128) ({} {}{}))'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tpl means 'template'.

It also might be good to use names in the string interpolation so its clear what each part is supposed to be.

Good point! I'd like to address this in a separate PR as it is a unified improvement.

cases.append(str(AssertReturn(case_data[0],
[self.v128_const(case_data[3][0], case_data[1][0]),
self.v128_const(case_data[3][1], case_data[1][1])],
self.v128_const(case_data[3][2], case_data[2][0]))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the previous data is an array, that's why it looks like unaligned.

(func (export "f64x2.max_with_const_24") (result v128) (f64x2.max (v128.const f64x2 0x00 0x01) (v128.const f64x2 0x00 0x01)))
(func (export "f64x2.max_with_const_25") (result v128) (f64x2.max (v128.const f64x2 0x02 0x80000000) (v128.const f64x2 0x02 0x80000000)))
;; f64x2.max param vs const
(func (export "f64x2.max_with_const_27")(param v128) (result v128) (f64x2.max (local.get 0) (v128.const f64x2 0 1)))
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 catch, I will fix it.

]

# Assert data
lst_oprt_with_const_assert = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means 'list of operator'. A data list specific for "const vs const" and "param vs const" tests. I will add more annotate here.

'max-abs': [
['-1.125'] * 2, ['0.125'] * 2, ['1.125'] * 2
]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These structures inherit from simd_arithmetic.py, specific for combination tests. Here should have annotate to explain this.

@arunetm arunetm merged commit 7437ebb into WebAssembly:master Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants