RESOLVED FIXED 118355
Remove code duplication from StringImpl create()/reallocate() methods
https://bugs.webkit.org/show_bug.cgi?id=118355
Summary Remove code duplication from StringImpl create()/reallocate() methods
Mikhail Pozdnyakov
Reported 2013-07-03 06:05:36 PDT
SSIA. StringImpl create()/reallocate() methods accepting LChar and UChar have duplicated code at the moment.
Attachments
patch (8.93 KB, patch)
2013-07-03 06:17 PDT, Mikhail Pozdnyakov
andersca: review+
patch v2 (8.99 KB, patch)
2013-07-04 04:06 PDT, Mikhail Pozdnyakov
no flags
patch v3 (9.42 KB, patch)
2013-07-04 05:53 PDT, Mikhail Pozdnyakov
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (847.34 KB, application/zip)
2013-07-04 08:05 PDT, Build Bot
no flags
patch v3 (9.42 KB, patch)
2013-07-04 08:25 PDT, Mikhail Pozdnyakov
no flags
Mikhail Pozdnyakov
Comment 1 2013-07-03 06:17:57 PDT
Mikhail Pozdnyakov
Comment 2 2013-07-03 06:19:45 PDT
Comment on attachment 205996 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review > Source/WTF/wtf/text/StringImpl.h:773 > + template <typename CharType> static PassRefPtr<StringImpl> constructInternal(StringImpl*, unsigned); may be it is better to keep these functions free, but some of them would need to become friends as accessing the class private fields.
Anders Carlsson
Comment 3 2013-07-03 08:29:14 PDT
Comment on attachment 205996 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review > Source/WTF/wtf/text/StringImpl.cpp:186 > +inline PassRefPtr<StringImpl> StringImpl::constructInternal(StringImpl* arena, unsigned length) I don't think arena is the right parameter name here. How about simply "stringImpl"? > Source/WTF/wtf/text/StringImpl.cpp:192 > +inline PassRefPtr<StringImpl> StringImpl::constructInternal<LChar>(StringImpl* arena, unsigned length) Ditto.
Mikhail Pozdnyakov
Comment 4 2013-07-03 08:39:47 PDT
Mikhail Pozdnyakov
Comment 5 2013-07-03 08:40:20 PDT
(In reply to comment #3) > (From update of attachment 205996 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review > > > Source/WTF/wtf/text/StringImpl.cpp:186 > > +inline PassRefPtr<StringImpl> StringImpl::constructInternal(StringImpl* arena, unsigned length) > > I don't think arena is the right parameter name here. How about simply "stringImpl"? > > > Source/WTF/wtf/text/StringImpl.cpp:192 > > +inline PassRefPtr<StringImpl> StringImpl::constructInternal<LChar>(StringImpl* arena, unsigned length) > > Ditto. Done.
Simon Fraser (smfr)
Comment 6 2013-07-03 09:59:57 PDT
JSCore tests are asserting after this change: ASSERTION FAILED: originalString->is8Bit() /Volumes/Data/slave/lion-debug/build/Source/WTF/wtf/text/StringImpl.cpp(230) : static PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal(PassRefPtr<WTF::StringImpl>, unsigned int, CharType *&) [CharType = unsigned short] 1 0x10fac5670 WTFCrash 2 0x10fb0397a WTF::PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal<unsigned short>(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&) 3 0x10fafb91a WTF::StringImpl::reallocate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&) 4 0x10faf9af9 void WTF::StringBuilder::reallocateBuffer<unsigned short>(unsigned int) 5 0x10fafb2ba unsigned short* WTF::StringBuilder::appendUninitializedSlow<unsigned short>(unsigned int) 6 0x10fafac19 unsigned short* WTF::StringBuilder::appendUninitialized<unsigned short>(unsigned int) 7 0x10fafa09a WTF::StringBuilder::append(unsigned char const*, unsigned int) 8 0x10f6e9ecd WTF::StringBuilder::append(unsigned char) 9 0x10f6e9c5f WTF::StringBuilder::append(char) 10 0x10f9193d9 JSC::Stringifier::appendQuotedString(WTF::StringBuilder&, WTF::String const&) 11 0x10f91a2bb JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&) 12 0x10f919292 JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&) 13 0x10f9189e9 JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>) 14 0x10f91b5e7 JSC::JSONStringify(JSC::ExecState*, JSC::JSValue, unsigned int) 15 0x10f9396c0 JSValueCreateJSONString http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29 Please fix or the change will be reverted.
WebKit Commit Bot
Comment 7 2013-07-03 10:02:28 PDT
Re-opened since this is blocked by bug 118361
Mikhail Pozdnyakov
Comment 8 2013-07-04 03:37:35 PDT
(In reply to comment #6) > JSCore tests are asserting after this change: > > ASSERTION FAILED: originalString->is8Bit() > /Volumes/Data/slave/lion-debug/build/Source/WTF/wtf/text/StringImpl.cpp(230) : static PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal(PassRefPtr<WTF::StringImpl>, unsigned int, CharType *&) [CharType = unsigned short] > 1 0x10fac5670 WTFCrash > 2 0x10fb0397a WTF::PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal<unsigned short>(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&) > 3 0x10fafb91a WTF::StringImpl::reallocate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&) > 4 0x10faf9af9 void WTF::StringBuilder::reallocateBuffer<unsigned short>(unsigned int) > 5 0x10fafb2ba unsigned short* WTF::StringBuilder::appendUninitializedSlow<unsigned short>(unsigned int) > 6 0x10fafac19 unsigned short* WTF::StringBuilder::appendUninitialized<unsigned short>(unsigned int) > 7 0x10fafa09a WTF::StringBuilder::append(unsigned char const*, unsigned int) > 8 0x10f6e9ecd WTF::StringBuilder::append(unsigned char) > 9 0x10f6e9c5f WTF::StringBuilder::append(char) > 10 0x10f9193d9 JSC::Stringifier::appendQuotedString(WTF::StringBuilder&, WTF::String const&) > 11 0x10f91a2bb JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&) > 12 0x10f919292 JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&) > 13 0x10f9189e9 JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>) > 14 0x10f91b5e7 JSC::JSONStringify(JSC::ExecState*, JSC::JSValue, unsigned int) > 15 0x10f9396c0 JSValueCreateJSONString > > http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29 > > Please fix or the change will be reverted. argh.. my bad, reallocate() methods should have input type-driven assertions: ASSERT(originalString->is8Bit()); and ASSERT(!originalString->is8Bit()); I missed it in my patch :(
Mikhail Pozdnyakov
Comment 9 2013-07-04 04:06:09 PDT
Created attachment 206072 [details] patch v2 Fixed the assertions.
Mikhail Pozdnyakov
Comment 10 2013-07-04 05:53:38 PDT
Created attachment 206082 [details] patch v3 Using constructInternal() also inside tryCreateUninitialized().
Build Bot
Comment 11 2013-07-04 08:05:32 PDT
Comment on attachment 206082 [details] patch v3 Attachment 206082 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/880891 New failing tests: compositing/backgrounds/fixed-backgrounds.html compositing/background-color/background-color-container.html
Build Bot
Comment 12 2013-07-04 08:05:36 PDT
Created attachment 206089 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Mikhail Pozdnyakov
Comment 13 2013-07-04 08:24:49 PDT
(In reply to comment #11) > (From update of attachment 206082 [details]) > Attachment 206082 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/880891 > > New failing tests: > compositing/backgrounds/fixed-backgrounds.html > compositing/background-color/background-color-container.html That looks really weird for WTF::String change..Guess it's not related.
Mikhail Pozdnyakov
Comment 14 2013-07-04 08:25:36 PDT
Created attachment 206092 [details] patch v3 run EWS.
Andreas Kling
Comment 15 2013-07-05 04:00:54 PDT
Comment on attachment 206092 [details] patch v3 Looks correct.
WebKit Commit Bot
Comment 16 2013-07-05 04:24:28 PDT
Comment on attachment 206092 [details] patch v3 Clearing flags on attachment: 206092 Committed r152415: <http://trac.webkit.org/changeset/152415>
WebKit Commit Bot
Comment 17 2013-07-05 04:24:32 PDT
All reviewed patches have been landed. Closing bug.
Benjamin Poulain
Comment 18 2013-07-16 20:32:23 PDT
The typename for characters should be CharacterType, not CharType.
Note You need to log in before you can comment on or make changes to this bug.