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+
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
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.