WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
patch v2
(8.99 KB, patch)
2013-07-04 04:06 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
patch v3
(9.42 KB, patch)
2013-07-04 05:53 PDT
,
Mikhail Pozdnyakov
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
patch v3
(9.42 KB, patch)
2013-07-04 08:25 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1
2013-07-03 06:17:57 PDT
Created
attachment 205996
[details]
patch
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
Committed
r152356
: <
http://trac.webkit.org/changeset/152356
>
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.
Top of Page
Format For Printing
XML
Clone This Bug