[JSC] Use new Apple ICU APIs to avoid C++ ICU API usage
Created attachment 424320 [details] Patch
<rdar://problem/75060240>
Created attachment 424321 [details] Patch
Comment on attachment 424321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424321&action=review > Source/JavaScriptCore/runtime/JSDateMath.cpp:107 > +#endif Why not just have an #else using OpaqueICUTimeZone = icu::Timezone; here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below?
Comment on attachment 424321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424321&action=review >> Source/JavaScriptCore/runtime/JSDateMath.cpp:107 >> +#endif > > Why not just have an #else > using OpaqueICUTimeZone = icu::Timezone; > here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below? That sounds nice. Changed.
Created attachment 424337 [details] Patch
Comment on attachment 424321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424321&action=review >>> Source/JavaScriptCore/runtime/JSDateMath.cpp:107 >>> +#endif >> >> Why not just have an #else >> using OpaqueICUTimeZone = icu::Timezone; >> here and avoid all the bitwise_cast, as well as remove some of the #if HAVE(ICU_C_TIMEZONE_API) below? > > That sounds nice. Changed. Ah, this does not work. using (typedef) and class forward declaration are different.
Created attachment 424339 [details] Patch
Comment on attachment 424339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424339&action=review r=me. Please make sure that the mac-AS-debug-wk2 bot failure is not real. > Source/JavaScriptCore/runtime/JSDateMath.cpp:133 > + // The above can fail if input date is invalid: NaN etc. > + // We can return any values in this case since later we fail when computing non timezone offset part anyway. > + constexpr LocalTimeOffset failed { false, 0 }; "The above" sounds wrong since there's no code that has any effect above. How about changing it to "This function"? This failed return value also seem to contradict the comment above the function which states that "If this function is called with NaN it returns NaN." Maybe remove the comment above the function?
Comment on attachment 424339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424339&action=review Thanks. And AS fail is media/media-fragments/TC0051.html, which is known flaky these days. >> Source/JavaScriptCore/runtime/JSDateMath.cpp:133 >> + constexpr LocalTimeOffset failed { false, 0 }; > > "The above" sounds wrong since there's no code that has any effect above. How about changing it to "This function"? > > This failed return value also seem to contradict the comment above the function which states that "If this function is called with NaN it returns NaN." Maybe remove the comment above the function? Yes, I've changed them.
Committed r275109 (235817@main): <https://commits.webkit.org/235817@main>
*** Bug 222160 has been marked as a duplicate of this bug. ***