Make a helper for the pattern of ICU functions that may need to be called twice to populate a buffer
Created attachment 398594 [details] Patch
Looking for feedback on the name of the function template, and also the other design considerations.
Comment on attachment 398594 [details] Patch Have to rebase and re-upload.
Created attachment 398617 [details] Patch
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());
Guess this is not working right yet. Tests are failing.
Created attachment 398624 [details] Patch
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.
Created attachment 398655 [details] Patch
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?
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!
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
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.
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.
Committed r261257: <https://trac.webkit.org/changeset/261257>
<rdar://problem/62949588>