Bug 211499

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

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>