[test][wasm] Migrate to new "addArray(type, {options})" syntax [v8/v8 : main]
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Another test cleanup. PTAL. I recommend starting the review in wasm-module-builder.js.
FYI, this patch was mostly generated by AI.
constructor(fields_or_options) {At first, I added `type_or_options` to `WasmArray`, then I realized that callers are less verbose if they don't have to spell out `type:`, so I changed it to the `type, options` pair you see below. Of course that now raises the question: would you like to review a follow-up where I apply the same tweak to WasmStruct? Concretely, that would mean
```
builder.addStruct({fields: [makeField(kWasmI32, true)], shared: true});
```
changes to
```
builder.addStruct([makeField(kWasmI32, true)], {shared: true});
```
at the cost of taking away the freedom of freely ordering the entries (it's conceivable that someone might e.g. want to define two struct types `{descriptor: B, fields: [...]}` and `{describes: A, fields: [...]}` where emphasizing their coupling as the first thing improves readability).
Related details
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 2 comments![]( "Open in Gerrit")
let mutability = true;Just to clarify, besides changing the signature this change also changes the default value?
How am I supposed to know whether there might be test cases that so far implicitly relied on the implicit `false` but didn't get updated in this change? 😊
let mutability = true;
let is_final = false;
let is_shared = false;
let supertype_idx = kNoSuperType;
if (typeof options === 'object') {
mutability = options.mutable ?? options.mutability ?? true;
is_final = options.is_final ?? options.final ?? false;
is_shared = options.is_shared ?? options.shared ?? false;
supertype_idx = MustBeNumber(
options.supertype_idx ?? options.supertype ?? kNoSuperType,
"supertype");
}This defines the default value multiple times which has some risk of surprising behavior.
Can't we simply do:
```suggestion
let mutability = options?.mutable ?? options?.mutability ?? true;
let is_final = options?.is_final ?? options?.final ?? false;
let is_shared = options?.is_shared ?? options?.shared ?? false;
let supertype_idx = MustBeNumber(
options?.supertype_idx ?? options?.supertype ?? kNoSuperType,
"supertype");
```
If you dislike it, we should refactor it in a different way that doesn't have separately defined defaults for an empty options object, e.g. use a default parameter `options = {}`.
Related details
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Auto-Submit | +1 |
3 comments
Comments addressed, PTAL.
I had also forgotten to update the testcase generator.
Just to clarify, besides changing the signature this change also changes the default value?
How am I supposed to know whether there might be test cases that so far implicitly relied on the implicit `false` but didn't get updated in this change? 😊
Yes, this changes the default for verbosity reasons: mutable arrays are much more useful, so we define them much more often.
If a test "relied" on the implicit default, didn't get updated, and still passes, then it didn't exactly "rely" on it, did it? 😊
But, fine, you're right, we should be careful. In a checkout before this patch, I added `if (typeof mutability === "undefined") throw "..."` to the module builder and ran the test suite, got 8 failures, and added explicit `{mutable: false}` to all of them (even though it technically doesn't matter).
let mutability = true;
let is_final = false;
let is_shared = false;
let supertype_idx = kNoSuperType;
if (typeof options === 'object') {
mutability = options.mutable ?? options.mutability ?? true;
is_final = options.is_final ?? options.final ?? false;
is_shared = options.is_shared ?? options.shared ?? false;
supertype_idx = MustBeNumber(
options.supertype_idx ?? options.supertype ?? kNoSuperType,
"supertype");
}This defines the default value multiple times which has some risk of surprising behavior.
Can't we simply do:
```suggestion
let mutability = options?.mutable ?? options?.mutability ?? true;
let is_final = options?.is_final ?? options?.final ?? false;
let is_shared = options?.is_shared ?? options?.shared ?? false;
let supertype_idx = MustBeNumber(
options?.supertype_idx ?? options?.supertype ?? kNoSuperType,
"supertype");
```
If you dislike it, we should refactor it in a different way that doesn't have separately defined defaults for an empty options object, e.g. use a default parameter `options = {}`.
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |