| Summary: | Make a helper for ICU functions that may need to be called twice to populate a buffer | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||
| Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, mifenton, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Darin Adler
2020-05-05 23:37:42 PDT
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> |