Bug 98299 – String::remove will convert an 8 bit string to a 16 bit string

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

The StringImpl::remove() method will up convert the contents of an 8 bit string and the result will always be a 16 bit string.

[Michael Saboff]

The remove method is actually in WTFString.cpp (String::remove).

[Michael Saboff]

Created attachment 166981 [details]
Patch

[Benjamin Poulain]

Comment on attachment 166981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166981&action=review

> Source/WTF/wtf/text/WTFString.cpp:281
>      if (static_cast<unsigned>(lengthToRemove) > length() - position)
>          lengthToRemove = length() - position;

No idea if that case is common.
If it is, shouldn't we just return a substring sharing impl here?

> Source/WTF/wtf/text/WTFString.cpp:289
> +        RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(length() - lengthToRemove, data);
> +        memcpy(data, characters8(), position * sizeof(LChar));
> +        memcpy(data + position, characters8() + position + lengthToRemove,
> +            (length() - lengthToRemove - position) * sizeof(LChar));
> +        m_impl = newImpl.release();

I would do an inline template with that code taking CharacterType as argument.

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 166981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166981&action=review
> 
> > Source/WTF/wtf/text/WTFString.cpp:281
> >      if (static_cast<unsigned>(lengthToRemove) > length() - position)
> >          lengthToRemove = length() - position;
> 
> No idea if that case is common.
> If it is, shouldn't we just return a substring sharing impl here?

From what I've seen, the use of remove is within length range and this wouldn't help.

> > Source/WTF/wtf/text/WTFString.cpp:289
> > +        RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(length() - lengthToRemove, data);
> > +        memcpy(data, characters8(), position * sizeof(LChar));
> > +        memcpy(data + position, characters8() + position + lengthToRemove,
> > +            (length() - lengthToRemove - position) * sizeof(LChar));
> > +        m_impl = newImpl.release();
> 
> I would do an inline template with that code taking CharacterType as argument.

Done.

[Michael Saboff]

Committed r130404: <http://trac.webkit.org/changeset/130404>