Skip to content

Commit

Permalink
[JSC] TypedArray sorting methods should have a special-case for compa…
Browse files Browse the repository at this point in the history
…rator returning `false`

https://bugs.webkit.org/show_bug.cgi?id=268075
<rdar://problem/122093956>

Reviewed by Yusuke Suzuki and Justin Michaud.

This change aligns TypedArray sorting methods with Array.prototype.sort() to treat `false`,
returned from comparator function, like smallest negative number rather than zero, which is a long-time
web reality quirk [1] to allow lexicographical comparison: `["c", "b", "a"].sort((a, b) => a > b)`.

The spec [2] still lacks normative steps on how to handle comparator return value in the common case.

This patch aligns JSC with V8 and SpiderMonkey.
However, it appears that V8 have recently removed the special-case for `false` in Array sorting methods.

[1]: https://bugs.webkit.org/show_bug.cgi?id=47825
[2]: https://tc39.es/ecma262/#sec-sortindexedproperties (step 4)

* JSTests/stress/sorting-boolean-result-comparator-typedarray.js: Added.
* Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::coerceComparatorResultToBoolean):
(JSC::genericTypedArrayViewProtoFuncSortImpl):

Canonical link: https://commits.webkit.org/276130@main
  • Loading branch information
Alexey Shvayka committed Mar 15, 2024
1 parent bee9faa commit 798d178
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 12 deletions.
16 changes: 16 additions & 0 deletions JSTests/stress/sorting-boolean-result-comparator-typedarray.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Bad value: ${actual}!`);
}

const makeArray = () => new Int8Array([10, 3, 8, 5, 30, 100, 6, 7, 100, 3]);
const expected = "100,100,30,10,8,7,6,5,3,3";

const comparator1 = (x, y) => x < y;
const comparator2 = comparator1.bind();
const comparator3 = new Proxy(comparator2, {});

shouldBe(makeArray().sort(comparator1).join(), expected);
shouldBe(makeArray().toSorted(comparator1).join(), expected);
shouldBe(makeArray().sort(comparator2).join(), expected);
shouldBe(makeArray().toSorted(comparator3).join(), expected);
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,20 @@ static ElementType* typedArrayMergeSort(VM& vm, Vector<ElementType, 16>& src, Ve
return from;
}

static ALWAYS_INLINE bool coerceComparatorResultToBoolean(JSGlobalObject* globalObject, ThrowScope& scope, JSValue comparatorResult)
{
if (LIKELY(comparatorResult.isInt32()))
return comparatorResult.asInt32() < 0;

// See https://bugs.webkit.org/show_bug.cgi?id=47825 on boolean special-casing
if (comparatorResult == jsBoolean(false))
return true;

double value = comparatorResult.toNumber(globalObject);
RETURN_IF_EXCEPTION(scope, { });
return value < 0;
}

template<typename ViewClass>
static ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncSortImpl(VM& vm, JSGlobalObject* globalObject, ViewClass* thisObject, JSValue comparatorValue)
{
Expand Down Expand Up @@ -821,13 +835,9 @@ static ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncSortImpl(VM& v

JSValue jsResult = cachedCall.call();
RETURN_IF_EXCEPTION(scope, { });

if (LIKELY(jsResult.isInt32()))
return jsResult.asInt32() < 0;

double value = jsResult.toNumber(globalObject);
bool result = coerceComparatorResultToBoolean(globalObject, scope, jsResult);
RETURN_IF_EXCEPTION(scope, { });
return value < 0;
return result;
});
RETURN_IF_EXCEPTION(scope, { });
} else {
Expand All @@ -851,13 +861,9 @@ static ALWAYS_INLINE EncodedJSValue genericTypedArrayViewProtoFuncSortImpl(VM& v

JSValue jsResult = call(globalObject, comparatorValue, callData, jsUndefined(), args);
RETURN_IF_EXCEPTION(scope, { });

if (LIKELY(jsResult.isInt32()))
return jsResult.asInt32() < 0;

double value = jsResult.toNumber(globalObject);
bool result = coerceComparatorResultToBoolean(globalObject, scope, jsResult);
RETURN_IF_EXCEPTION(scope, { });
return value < 0;
return result;
});
RETURN_IF_EXCEPTION(scope, { });
}
Expand Down

0 comments on commit 798d178

Please sign in to comment.