118355 – Remove code duplication from StringImpl create()/reallocate() methods

Bug 118355 - Remove code duplication from StringImpl create()/reallocate() methods

Summary: Remove code duplication from StringImpl create()/reallocate() methods

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Web Template Framework (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mikhail Pozdnyakov

URL:
Keywords:

Depends on:	118361
Blocks:
	Show dependency tree / graph

Reported:	2013-07-03 06:05 PDT by Mikhail Pozdnyakov
Modified:	2013-07-16 20:32 PDT (History)
CC List:	9 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mikhail Pozdnyakov 2013-07-03 06:05:36 PDT

SSIA. StringImpl create()/reallocate() methods accepting LChar and UChar have duplicated code at the moment.

Comment 1 Mikhail Pozdnyakov 2013-07-03 06:17:57 PDT

Created attachment 205996 [details]
patch

Comment 2 Mikhail Pozdnyakov 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.

Comment 3 Anders Carlsson 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.

Comment 4 Mikhail Pozdnyakov 2013-07-03 08:39:47 PDT

Committed r152356: <http://trac.webkit.org/changeset/152356>

Comment 5 Mikhail Pozdnyakov 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.

Comment 6 Simon Fraser (smfr) 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.

Comment 7 WebKit Commit Bot 2013-07-03 10:02:28 PDT

Re-opened since this is blocked by bug 118361

Comment 8 Mikhail Pozdnyakov 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 :(

Comment 9 Mikhail Pozdnyakov 2013-07-04 04:06:09 PDT

Created attachment 206072 [details]
patch v2

Fixed the assertions.

Comment 10 Mikhail Pozdnyakov 2013-07-04 05:53:38 PDT

Created attachment 206082 [details]
patch v3

Using constructInternal() also inside tryCreateUninitialized().

Comment 11 Build Bot 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

Comment 12 Build Bot 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

Comment 13 Mikhail Pozdnyakov 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.

Comment 14 Mikhail Pozdnyakov 2013-07-04 08:25:36 PDT

Created attachment 206092 [details]
patch v3

run EWS.

Comment 15 Andreas Kling 2013-07-05 04:00:54 PDT

Comment on attachment 206092 [details]
patch v3

Looks correct.

Comment 16 WebKit Commit Bot 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>

Comment 17 WebKit Commit Bot 2013-07-05 04:24:32 PDT

All reviewed patches have been landed.  Closing bug.

Comment 18 Benjamin Poulain 2013-07-16 20:32:23 PDT

The typename for characters should be CharacterType, not CharType.