Remove UChar* accessors from StringImpl

• Component: New Bugs
• Status: RESOLVED INVALID

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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 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]

http://trac.webkit.org/changeset/37538

[Darin Adler]

Closed the wrong bug by accident.

[Eric Seidel (no email)]

(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)]

I've decided this is not worth pursuing at this time.  Closing.