WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED INVALID
Bug 20065
Remove UChar* accessors from StringImpl
https://bugs.webkit.org/show_bug.cgi?id=20065
Summary
Remove UChar* accessors from StringImpl
Eric Seidel (no email)
Reported
2008-07-16 15:31:29 PDT
Remove UChar* accessors from StringImpl We should instead replace them with use of inline UChar charAt(unsigned) -like functions. This will promote better logic encapsulation and allow us to play with alternative string storage (for memory savings) w/o having to change several hundred call sites. If I find time I'll post a patch to do this.
Attachments
First attempt, push logic from String.cpp into StringImpl.cpp
(14.92 KB, patch)
2008-07-17 09:27 PDT
,
Eric Seidel (no email)
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2008-07-17 09:27:00 PDT
Created
attachment 22340
[details]
First attempt, push logic from String.cpp into StringImpl.cpp WebCore/ChangeLog | 26 ++++++++ WebCore/platform/text/PlatformString.h | 2 + WebCore/platform/text/String.cpp | 79 ++++++------------------- WebCore/platform/text/StringImpl.cpp | 104 +++++++++++++++++++++++++++++--- WebCore/platform/text/StringImpl.h | 12 ++++- 5 files changed, 153 insertions(+), 70 deletions(-)
Darin Adler
Comment 2
2008-08-21 17:51:13 PDT
Comment on
attachment 22340
[details]
First attempt, push logic from String.cpp into StringImpl.cpp These changes are OK, but I don't think they really free us up to change the implementation. There are clients of String::characters() and for good reason. 102 // String::append and insert are extremely in-efficient, if you're doing many 103 // appends and inserts, consider using a Vector<UChar> or SegmetedString inefficient doesn't have a "-" in it. SegmentedString has an "n" in it. 162 m_impl = m_impl->insert(charactersToInsert, lengthToInsert, position); Why did you remove the ASSERT(m_impl) from this function? I noticed that StringImpl::append(StringImpl*) does not optimize the zero-length case. The old code didn't either, but that's not good. I also think it's a bit strange to use codePointAt everywhere instead of the [] operator. r=me
Darin Adler
Comment 3
2008-10-12 17:34:33 PDT
http://trac.webkit.org/changeset/37538
Darin Adler
Comment 4
2008-10-12 17:34:48 PDT
Closed the wrong bug by accident.
Eric Seidel (no email)
Comment 5
2008-12-02 11:36:09 PST
(In reply to
comment #2
)
> (From update of
attachment 22340
[details]
[review]) > These changes are OK, but I don't think they really free us up to change the > implementation. There are clients of String::characters() and for good reason.
Yeah, I kinda lost interest in this change after making it. But I'm finally cleaning it up today to land. I think there were some nice clean-ups in it, making it worth landing.
> 102 // String::append and insert are extremely in-efficient, if you're > doing many > 103 // appends and inserts, consider using a Vector<UChar> or > SegmetedString > > inefficient doesn't have a "-" in it. > SegmentedString has an "n" in it.
Fixed.
> 162 m_impl = m_impl->insert(charactersToInsert, lengthToInsert, position); > > Why did you remove the ASSERT(m_impl) from this function?
Shouldn't have. Fixed.
> I noticed that StringImpl::append(StringImpl*) does not optimize the > zero-length case. The old code didn't either, but that's not good.
Fixed.
> I also think it's a bit strange to use codePointAt everywhere instead of the [] > operator.
Felt clearer in my head at the time at least.
Eric Seidel (no email)
Comment 6
2008-12-16 13:48:06 PST
I've decided this is not worth pursuing at this time. Closing.
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