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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has asked  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 26618: Addressed comments -- Part 2: Refactor UString::Rep into two
classes which reduces memory usage for the base Rep class.
https://bugs.webkit.org/attachment.cgi?id=26618&action=review

------- Additional Comments from David Levin <levin at chromium.org>

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

> > +}	// namespace JSC
> > +
> > +#endif  // SmallStrings_h
> I suggest only one space before "//" to match the style elsewhere, rather
than two spaces.
Fixed.

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

rep has just been recreated, so the local variable base is no longer valid at
this point.


> > +		 UChar* data() const;
> I'm surprised this can be non-inline without hurting performance slightly.

Still inlined just not defined in the class definition.


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

Fixed.

> > +		 static BaseString& empty() { return *emptyBaseString; }
> > +	     private:
> > +		 friend void initializeUString();
> I'd prefer a blank line before the private line.

Fixed.

> +    inline UChar* UString::Rep::data() const
> +    {
> +	   const UString::BaseString* base = baseString();
> Do you need the UString:: qualifier on BaseString here?

Fixed.


> > +	     UString::BaseString* base = m_rep->baseString();
>And here?
Fixed.


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

and

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

and

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

In the next change, I'll make this point to something else if it is a
BaseString.  This is the reason for the flag and putting 0 in there in the case
of the BaseString and for making a it void* that no code can use directly.

Why re-use this field? In order to not make BaseString any bigger.  (I've been
under the impression that this is desired.)