Bug 211499 – Make a helper for ICU functions that may need to be called twice to populate a buffer

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Make a helper for the pattern of ICU functions that may need to be called twice to populate a buffer

[Darin Adler]

Created attachment 398594 [details]
Patch

[Darin Adler]

Looking for feedback on the name of the function template, and also the other design considerations.

[Darin Adler]

Comment on attachment 398594 [details]
Patch

Have to rebase and re-upload.

[Darin Adler]

Created attachment 398617 [details]
Patch

[Darin Adler]

Comment on attachment 398617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398617&action=review

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1614
> +    Vector<UChar> buffer(s.length());

On reflection, I think I’ll change this to:

    Vector<UChar> buffer;
    buffer.reserveInitialCapacity(s.length());

[Darin Adler]

Guess this is not working right yet. Tests are failing.

[Darin Adler]

Created attachment 398624 [details]
Patch

[Darin Adler]

Interesting, looks like one of the JSC stress tests is failing:

    stress/intl-relativetimeformat.js.intl-relativetimeformat-enabled

I don’t know much about how to investigate and debug; have to figure that out.

[Darin Adler]

Created attachment 398655 [details]
Patch

[Ross Kirsling]

Comment on attachment 398655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398655&action=review

r=me pending EWS, this is really cool.

> Source/WTF/ChangeLog:10
> +        specia considerations because of null character terminatorion, or destinations that are

spelling: specia, terminatorion

> Source/WTF/wtf/unicode/icu/ICUHelpers.h:77
> +template<typename FirstArgumentType, typename ...OtherArgumentTypes> auto argumentTuple(FirstArgumentType&&, OtherArgumentTypes&&...);

How come this declaration is needed for argumentTuple but not for findVector?

[Darin Adler]

Comment on attachment 398655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398655&action=review

>> Source/WTF/ChangeLog:10
>> +        specia considerations because of null character terminatorion, or destinations that are
> 
> spelling: specia, terminatorion

OK, I’ll fix those.

>> Source/WTF/wtf/unicode/icu/ICUHelpers.h:77
>> +template<typename FirstArgumentType, typename ...OtherArgumentTypes> auto argumentTuple(FirstArgumentType&&, OtherArgumentTypes&&...);
> 
> How come this declaration is needed for argumentTuple but not for findVector?

Must admit I am not 100% sure. Without it, the code does not compile, though!

[Saam Barati]

Comment on attachment 398655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398655&action=review

LGTM too

> Source/WTF/wtf/unicode/icu/ICUHelpers.h:81
> +    return tuple_cat(std::make_tuple(buffer.data(), buffer.size()), argumentTuple(std::forward<OtherArgumentTypes>(otherArguments)...));

Nice

> Source/WTF/wtf/unicode/icu/ICUHelpers.h:91
> +template<typename FunctionType, typename ...ArgumentTypes> UErrorCode callBufferProducingFunction(const FunctionType& function, ArgumentTypes&&... arguments)

This is nice. I like how you solved passing in a Vector

[Darin Adler]

Comment on attachment 398655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398655&action=review

>>> Source/WTF/wtf/unicode/icu/ICUHelpers.h:77
>>> +template<typename FirstArgumentType, typename ...OtherArgumentTypes> auto argumentTuple(FirstArgumentType&&, OtherArgumentTypes&&...);
>> 
>> How come this declaration is needed for argumentTuple but not for findVector?
> 
> Must admit I am not 100% sure. Without it, the code does not compile, though!

Oh, I figured it out. The difference is that the Vector overload of findVector never recursively calls the other findVector; just returns the reference to the vector. But in the case of argumentTuple, the overload that handles the Vector *does* recursively call argumentTuple. And to resolve it correctly it apparently needs to have already seen the template declaration.

[Darin Adler]

Comment on attachment 398655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398655&action=review

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:622
> -    Vector<UChar, 32> patternBuffer(32);
> -    status = U_ZERO_ERROR;
> -    auto patternLength = udatpg_getBestPatternWithOptions(generator, skeletonView.upconvertedCharacters(), skeletonView.length(), UDATPG_MATCH_HOUR_FIELD_LENGTH, patternBuffer.data(), patternBuffer.size(), &status);
> -    if (needsToGrowToProduceBuffer(status)) {
> -        status = U_ZERO_ERROR;
> -        patternBuffer.grow(patternLength);
> -        udatpg_getBestPattern(generator, skeletonView.upconvertedCharacters(), skeletonView.length(), patternBuffer.data(), patternLength, &status);
> -    }
> +    Vector<UChar, 32> patternBuffer;
> +    status = callBufferProducingFunction(udatpg_getBestPatternWithOptions, generator, skeletonView.upconvertedCharacters(), skeletonView.length(), UDATPG_MATCH_HOUR_FIELD_LENGTH, patternBuffer);

I just noticed a behavior change here.

The old code called udatpg_getBestPatternWithOptions first, but then called udatpg_getBestPattern the second time. I suspect the refactoring fixed a potential bug.

[Darin Adler]

Committed r261257: <https://trac.webkit.org/changeset/261257>

[Radar WebKit Bug Importer]

<rdar://problem/62949588>