Fix String.prototype.localeCompare on xplat when Intl is enabled #4187
Conversation
|
Can we have a test case for this? |
|
Test cases for localeCompare are a known issue -- I was planning on adding them in a future revision to our existing Intl.Collator tests |
|
If that’s planned already, this PR looks like a good opportunity to add one. Besides, if we could open an issue on Github, individuals could contribute test cases. |
|
I can make a github issue for it, but I believe the intl tests have changed format a bit between release/1.7 and current master. |
Unfortunately this is a reality we have to deal with, and is not a reason to skip adding tests. |
| { | ||
| IntlEngineInterfaceExtensionObject* intlExtensionObject = static_cast<IntlEngineInterfaceExtensionObject*>(nativeEngineInterfaceObj->GetEngineExtension(EngineInterfaceExtensionKind_Intl)); | ||
| IntlEngineInterfaceExtensionObject* intlExtensionObject = static_cast<IntlEngineInterfaceExtensionObject*>( | ||
| nativeEngineInterfaceObj->GetEngineExtension(EngineInterfaceExtensionKind_Intl)); |
There was a problem hiding this comment.
Is this just breaking the line?
There was a problem hiding this comment.
Yeah, I felt it was too long.
| CallInfo toPass(callInfo.Flags, 7); | ||
| return intlExtensionObject->EntryIntl_CompareString(function, toPass, undefined, pThis, pThat, undefined, undefined, undefined, undefined); | ||
| return CALL_ENTRYPOINT(scriptContext->GetThreadContext(), intlExtensionObject->EntryIntl_CompareString, function, toPass, | ||
| undefined, pThis, pThat, undefined, undefined, undefined, undefined); |
There was a problem hiding this comment.
nit: have the params to intlExtensionObject->EntryIntl_CompareString all together on the second line.
There was a problem hiding this comment.
@rajatd Does CALL_ENTRYPOINT need to be wrapped in implicit call like the uses in JavascriptOperators.cpp (like CallGetter)?
Pattern:
...
return threadContext->ExecuteImplicitCall(function, ImplicitCall_Accessor, [=]() -> Js::Var
{
...
Var result = CALL_ENTRYPOINT(...)
result = CrossSite::MarshalVar(requestContext, result);
return result;
});
/cc @akroshg
There was a problem hiding this comment.
@jackhorton I think some carefully-designed tests with a loop and -mic:1 might reveal JIT-related failures.
There was a problem hiding this comment.
@dilijev I am not sure what careful design would need to go in here to execute anything like that. Also, keep in mind that Intl is not enabled in xplat CI (or else we would have seen this, most likely, from the single test we have that calls .localeCompare), so tests stressing it wont reveal much for now (this bug is xplat-only because CALL_ENTRYPOINT is only "needed" on xplat, modulo your concern about implicit calls.
7a99ef9 to
ee826f2
Compare
| return intlExtensionObject->EntryIntl_CompareString(function, toPass, undefined, pThis, pThat, undefined, undefined, undefined, undefined); | ||
| ThreadContext *threadContext = scriptContext->GetThreadContext(); | ||
| return threadContext->ExecuteImplicitCall(function, ImplicitCall_Accessor, | ||
| [scriptContext, threadContext, intlExtensionObject, function, toPass, undefined, pThis, pThat]() -> Var |
There was a problem hiding this comment.
Quick note that you're capturing all of these variables by copy. Consider whether capture-by-reference would be better for any of these. Also, unless you feel that it's important to limit or enumerate the list of captures, you can capture everything (that is actually used) with [=] (copy) or [&] (reference).
There was a problem hiding this comment.
Also looks like you don't need scriptContext at all.
There was a problem hiding this comment.
I didn't want to capture the pointers by reference, and i think most/all of these are pointers. With that being said, I didnt want to capture everything by copy since I didn't necessarily know which copy constructors would run as a result. You're right that I don't need scriptContext anymore, i did before.
ee826f2 to
399b6e8
Compare
|
Superseded by #4217 |
No description provided.