[immutable-arraybuffer] TypedArray internal operations#4551
Conversation
55e9cb7 to
795f516
Compare
795f516 to
0fa24b7
Compare
0fa24b7 to
c8acdac
Compare
|
Needs a non-trivial rebase. |
c8acdac to
3be710d
Compare
Done. |
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
I haven't gotten most of the way through this yet, but here are some comments. I also don't understand what the purpose is of the first three commits - the first one ("Make better use of testTypedArray.js") seems like it's actually removing coverage in some cases?
9440796 to
dea78c5
Compare
…rray method tests
…perties cannot be set when the backing buffer is immutable
…es are non-configurable and non-writable when the backing buffer is immutable
…es being defined are validated against descriptors for immutable properties
dea78c5 to
8fb816f
Compare
|
The linter failure seems to be a side effect of my recent fix of harness file |
|
(Removing |
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
I've convinced myself that the new tests cover the remaining unchecked items in #4509, so this works for me. Feel free to merge when the comments are addressed, as far as I'm concerned.
I only skimmed the files where the TypedArray arg factories were edited.
It occurred to me that it'd be nice to add harness tests for the bugs you fixed in verifyProperty, but I'd say that's optional too.
| function makeSpy(calls, label, value) { | ||
| return { | ||
| toString() { | ||
| calls.push(label + " toString"); | ||
| return value; | ||
| }, | ||
| valueOf() { | ||
| calls.push(label + " valueOf"); | ||
| return value; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
optional: You could use TemporalHelpers.toPrimitiveObserver for this. Although, we really need to move that into its own helper file. (Same comment for files in the same commit)
| function unloggedHasOwn(obj, key, calls) { | ||
| var length = calls.length; | ||
| var result = obj.hasOwnProperty(key); | ||
| calls.length = length; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
If I'm reading correctly, this won't log anything, regardless? In that case it seems better to just use hasOwnProperty and only add this in the future if it's necessary. (Same comment for files in the same commit)
| assert.deepEqual(Object.getOwnPropertyDescriptor(sample, "0"), descriptor, | ||
| "results of Reflect.defineProperty " + reprDesc(newDesc)); |
There was a problem hiding this comment.
optional: I'd like to avoid assert.deepEqual in new tests (#3476) but this PR has been waiting long enough so I wouldn't hold things up on that.
If you do feel like fixing it, I think at first glance verifyProperty(sample, "0", descriptor, { label: ..., restore: true }) ought to work? But I haven 't looked at that too closely. If not, then a custom comparison function seems like the way to go.
| // As a special case, never decrease array length (which would delete | ||
| // properties not covered by the new length). | ||
| if (oldValue <= 0 || __isArray(obj) && name === "length") { | ||
| newValue = oldValue === -Infinity ? 0 : oldValue + 1; | ||
| } else { | ||
| newValue = oldValue === Infinity ? 1 : oldValue - 1; | ||
| } |
There was a problem hiding this comment.
I think this produces a false positive for readonly NaN properties, because newValue is also NaN, whereas the old code would've written "unlikelyValue" and correctly determined that the write failed. Verified with a spot check:
var obj = {};
Object.defineProperty(obj, "a", { value: NaN, writable: false });
assert(verifyProperty(obj, 'a', { value: NaN, writable: false }));3 participants
Ref #4509
Most of the changes just relax filters applied to
testWithTypedArrayConstructorsand similar harness/testTypedArray.js exports such that immutable ArrayBuffers are no longer excluded.