[webkit-reviews] review denied: [Bug 23175] String and UString should be able to share a UChar* buffer. : [Attachment 26609] Part 2: Refactor UString::Rep into two classes which reduces memory usage for the base Rep class.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 23175: String and UString should be able to share a UChar* buffer.
https://bugs.webkit.org/show_bug.cgi?id=23175

Attachment 26609: Part 2: Refactor UString::Rep into two classes which reduces
memory usage for the base Rep class.
https://bugs.webkit.org/attachment.cgi?id=26609&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This looks good.

> +static const unsigned numCharactersToStore = 0x100; // Note that
SmallStrings::m_singleCharacterStrings should be the same size.

We have COMPILE_ASSERT so that such things can be checked rather than just
mentioned in comments.

> +}  // namespace JSC
> +
> +#endif  // SmallStrings_h

If the patch is going to touch some files just to add comments, then I will be
in full nitpick mode. I suggest only one space before "//" to match the style
elsewhere, rather than two spaces.

> -	       rep->capacity = newCapacity;
> +	       rep->baseString()->capacity = newCapacity;

I would have expected you to use "base" here rather than re-fetching
rep->baseString(). Is this better in some way?

> -	       bool baseIsSelf() const { return baseString == this; }
> +	       bool baseIsSelf() const { return
m_identifierTableAndFlags.isFlagSet(BaseStringFlag); }

Why can't we keep the old definition, but move it down in the file where it can
see the BaseString class definition? Or can't this check if baseString is zero
instead? Do we really need the bit in the flags? Does the bit help performance
or something?

> -	       UChar* data() const { return baseString->buf +
baseString->preCapacity + offset; }
> +	       UChar* data() const;

I'm surprised this can be non-inline without hurting performance slightly.

> +	       void setBaseString(PassRefPtr<BaseString> base) { ASSERT(base !=
this); m_baseString = base.releaseRef(); }
> +	       BaseString* baseString() { return
reinterpret_cast<BaseString*>(baseIsSelf() ? this : m_baseString); }
> +	       const BaseString* baseString() const { return const_cast<const
BaseString*>(const_cast<Rep*>(this)->baseString());  }

It's annoying to have these reinterpret_cast here. Instead you should just
define these inline functions outside the class definition, in a place that can
see the definition of BaseString.

> +	       void* m_baseString; // If "this" is a BaseString instance, it is
0. BaseString* otherwise.

Seems really ugly to have this be a void*. Can't it be a BaseString* instead?
When is the fact that it's a void* helpful?

> +
> +	       static BaseString& null() { return *nullBaseString; }
> +	       static BaseString& empty() { return *emptyBaseString; }
> +	   private:
> +	       friend void initializeUString();

I'd prefer a blank line before the private line.

> +    inline UChar* UString::Rep::data() const
> +    {
> +	   const UString::BaseString* base = baseString();
> +	   return base->buf + base->preCapacity + offset;
> +    }

Do you need the UString:: qualifier on BaseString here?

> +	   UString::BaseString* base = m_rep->baseString();

And here?

I'm going to say review- because of the reinterpret_cast question, and the
question of whether we really do need a bit to tell whether something is a base
string. But this does seem like a great change and about ready to land.