Issue: <https://github.com/tc39/proposal-promise-any/issues/38> PR: <https://github.com/tc39/proposal-promise-any/pull/64>
Created attachment 400872 [details] Patch
Comment on attachment 400872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400872&action=review r=me with comments > Source/JavaScriptCore/runtime/AggregateError.cpp:48 > + putDirectWithoutTransition(vm, vm.propertyNames->errors, constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors), static_cast<unsigned>(PropertyAttribute::DontEnum)); 1. constructArray can throw an error potentially (e.g. OutOfMemory error in extreme condition). So, let's perform error check. auto scope = DECLARE_THROW_SCOPE(vm); JSArray* errors = constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors); RETURN_IF_EXCEPTION(scope, void()); 2. Use putDirect instead of putDirectWithoutTransition. putXXXWithoutTransition will put this name to Structure without structure transition. So, putXXXWithoutTransition is only allowed for objects which are created per-JSGlobalObject etc. For example, we create one AggregateError with Structure A. This structure A will have "errors" name since we are not performing structure transition. But this A will be used for another AggregateError even if it was not having "errors" name yet. > Source/JavaScriptCore/runtime/AggregateError.h:61 > return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info()); Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace. > Source/JavaScriptCore/runtime/AggregateError.h:78 > Let's put `STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(AggregateError, AggregateError::Base);` to ensure that AggregateError can share IsoSubspace with ErrorInstance.
Comment on attachment 400872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400872&action=review >> Source/JavaScriptCore/runtime/AggregateError.cpp:48 >> + putDirectWithoutTransition(vm, vm.propertyNames->errors, constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors), static_cast<unsigned>(PropertyAttribute::DontEnum)); > > 1. constructArray can throw an error potentially (e.g. OutOfMemory error in extreme condition). So, let's perform error check. > > auto scope = DECLARE_THROW_SCOPE(vm); > JSArray* errors = constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors); > RETURN_IF_EXCEPTION(scope, void()); > > 2. Use putDirect instead of putDirectWithoutTransition. putXXXWithoutTransition will put this name to Structure without structure transition. So, putXXXWithoutTransition is only allowed for objects which are created per-JSGlobalObject etc. > For example, we create one AggregateError with Structure A. This structure A will have "errors" name since we are not performing structure transition. But this A will be used for another AggregateError even if it was not having "errors" name yet. Ah! I hadn't thought of the "But this A will be used for another AggregateError even if it was not having "errors" name yet." situation. Thanks for the detailed explanation :) >> Source/JavaScriptCore/runtime/AggregateError.h:61 >> return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info()); > > Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace. Neat! >> Source/JavaScriptCore/runtime/AggregateError.h:78 >> > > Let's put `STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(AggregateError, AggregateError::Base);` to ensure that AggregateError can share IsoSubspace with ErrorInstance. Also Neat!
Comment on attachment 400872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400872&action=review >>> Source/JavaScriptCore/runtime/AggregateError.h:61 >>> return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info()); >> >> Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace. > > Neat! Ah, and you can also remove destroy function.
Created attachment 400878 [details] Patch i'm going to wait for test262 to update before landing this
Can we just land it with (almost) copied test262 tests?
(In reply to Yusuke Suzuki from comment #6) > Can we just land it with (almost) copied test262 tests? AFAICT, this change would cause to fail a few existing test262 tests and I have yet to see any issues/PRs that would update them. Here's all the related issues/PRs that I've been able to find: - AggregateError Updates <https://github.com/tc39/test262/issues/2646> - AggregateError updates. Ref gh-2646 <https://github.com/tc39/test262/pull/2647> neither of which change the tests from checking `AggregateError.prototype.errors` to checking for an own property on any `AggregateError` instance. I can write some super simple tests for this in the meantime.
Created attachment 401815 [details] Patch
Created attachment 401816 [details] Patch oops, forgot the "Reviewed by"
Committed r263006: <https://trac.webkit.org/changeset/263006> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401816 [details].
<rdar://problem/64332285>