| Summary: | [JSC] Use fastJoin in Array#toString | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, buildbot, cdumez, cmarcelo, darin, dbates, fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2017-10-07 23:41:08 PDT
Created attachment 323124 [details]
Patch
Created attachment 323125 [details]
Patch
Comment on attachment 323125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323125&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672 > + if (element.isUndefinedOrNull()) > + continue; Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage? Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences. > Source/JavaScriptCore/runtime/JSStringJoiner.h:41 > + void appendInt32(VM&, int32_t); > + void appendDouble(VM&, double); I probably would have used overloading and named these both appendNumber or appendNumeral. I believe that if someone used it with any other type besides int32_t and double, it would not compile because the overloading would be ambiguous. That’s how the underlying numericStrings.add works. But your way is clear and explicit so I am OK with it. Comment on attachment 323125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323125&action=review >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672 >> + continue; > > Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage? > > Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences. On reflection, obviously the patch doesn’t change anything because this is toLocaleString, not toString, but still this is strange. Comment on attachment 323125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323125&action=review Thank you! >>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:672 >>> + continue; >> >> Why does the ENABLE(INTL) version put in extra commas, but the !ENABLE(INTL) version doesn’t? There is no stringJoiner.appendEmptyString() here. Does your patch change behavior in one of the two cases? Do we have test coverage? >> >> Can we put ENABLE(INTL) around less of the function? It seems like the code is *almost* identical. There are only three differences. > > On reflection, obviously the patch doesn’t change anything because this is toLocaleString, not toString, but still this is strange. Yeah, this patch does not change toLocaleString. But right, it looks incorrect. According to the spec[1], we should take ENABLE(INTL) version's behavior. I'll clean up this part and add a test. [1]: https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring >> Source/JavaScriptCore/runtime/JSStringJoiner.h:41 >> + void appendDouble(VM&, double); > > I probably would have used overloading and named these both appendNumber or appendNumeral. I believe that if someone used it with any other type besides int32_t and double, it would not compile because the overloading would be ambiguous. That’s how the underlying numericStrings.add works. > > But your way is clear and explicit so I am OK with it. OK, I'll use overloading version since the underlying numericStrings.add follows this. I'll change them to `appendNumber`. Committed r223834: <https://trac.webkit.org/changeset/223834> |