| Summary: | Introduce makeAtomString() | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
| Component: | Web Template Framework | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, changseok, cmarcelo, darin, dino, esprehn+autocc, ews-watchlist, fmalita, gyuyoung.kim, pdr, sabouhallawa, sam, schenney, sergio, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=239427 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Chris Dumez
2022-04-18 14:09:29 PDT
Created attachment 457820 [details]
Patch
Created attachment 457824 [details]
Patch
Comment on attachment 457824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457824&action=review > Source/WTF/wtf/text/StringConcatenate.h:502 > + if (areAllAdapters8Bit) { > + LChar buffer[maxLengthToUseStackVariable]; > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > + return AtomString { buffer, length }; > + } > + UChar buffer[maxLengthToUseStackVariable]; > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > + return AtomString { buffer, length }; I wonder if the compiler is smart enough to not allocate both buffers. > Source/WTF/wtf/text/StringConcatenate.h:525 > + if (areAllAdapters8Bit) { > + LChar* buffer; > + RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer); > + if (!resultImpl) > + return nullAtom(); > + > + if (buffer) > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > + > + return resultImpl.get(); > + } > + > + UChar* buffer; > + RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer); > + if (!resultImpl) > + return nullAtom(); > + > + if (buffer) > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > + > + return resultImpl.get(); Is there a way to share code with tryMakeStringFromAdapters here? Created attachment 457836 [details]
Patch
Created attachment 457837 [details]
Patch
Created attachment 457838 [details]
Patch
Comment on attachment 457838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457838&action=review > Source/WebCore/svg/SVGLengthValue.cpp:30 > +#include <wtf/text/StringConcatenate.h> > #include <wtf/text/StringConcatenateNumbers.h> I thought "StringConcatenateNumbers.h" included "StringConcatenate.h" (In reply to Darin Adler from comment #7) > Comment on attachment 457838 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457838&action=review > > > Source/WebCore/svg/SVGLengthValue.cpp:30 > > +#include <wtf/text/StringConcatenate.h> > > #include <wtf/text/StringConcatenateNumbers.h> > > I thought "StringConcatenateNumbers.h" included "StringConcatenate.h" Oh, I think that was me trying to fix a build error the wrong way. I'll drop. Created attachment 457841 [details]
Patch
(In reply to Darin Adler from comment #3) > Comment on attachment 457824 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457824&action=review > > > Source/WTF/wtf/text/StringConcatenate.h:502 > > + if (areAllAdapters8Bit) { > > + LChar buffer[maxLengthToUseStackVariable]; > > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > > + return AtomString { buffer, length }; > > + } > > + UChar buffer[maxLengthToUseStackVariable]; > > + stringTypeAdapterAccumulator(buffer, adapter, adapters...); > > + return AtomString { buffer, length }; > > I wonder if the compiler is smart enough to not allocate both buffers. Good question. I am not quite sure how to find out though. Comment on attachment 457841 [details]
Patch
Very nice.
Committed r293024 (249763@main): <https://commits.webkit.org/249763@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457841 [details]. |