Bug 20065 - Remove UChar* accessors from StringImpl
Summary: Remove UChar* accessors from StringImpl
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-16 15:31 PDT by Eric Seidel (no email)
Modified: 2008-12-16 13:48 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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.
Comment 1 Eric Seidel (no email) 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(-)
Comment 2 Darin Adler 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
Comment 3 Darin Adler 2008-10-12 17:34:33 PDT
http://trac.webkit.org/changeset/37538
Comment 4 Darin Adler 2008-10-12 17:34:48 PDT
Closed the wrong bug by accident.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Eric Seidel (no email) 2008-12-16 13:48:06 PST
I've decided this is not worth pursuing at this time.  Closing.