[webkit-reviews] review denied: [Bug 30141] Make AtomicString construct StringImpls in single malloc blocks : [Attachment 40887] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 30141: Make AtomicString construct StringImpls in single malloc blocks
https://bugs.webkit.org/show_bug.cgi?id=30141

Attachment 40887: patch
https://bugs.webkit.org/attachment.cgi?id=40887&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Make AtomicString create its StringImpl via create(), not the
constructor,
> +	   so it gets allocated in a single heap block, saving memory and CPU
cycles.

Beautiful!

> +	   This eliminates two StringImpl constructors, making the remaining
ones
> +	   unambiguous, so the "AdoptBuffer" parameter is no longer needed.

The behavior of this constructor is a bit unconventional because it takes
ownership of a buffer, but I guess that's OK because it's private. We should
have a comment in the header making it clear that it adopts the buffer, though;
this would be completely unclear if reading the header.

> +	   Added const attribute to UChar* in StringImpl constructor,
eliminating the
> +	   need for several const_casts in calls to it.

Great!

It is a bit strange to have a function take ownership of a buffer but be given
a const UChar* to it. But I guess the comment could clarify that sufficiently.

> +	   StringImpl also unfriends AtomicString (OMG drama!!!)

Superb!

But since nobody else needs access to the setHash or setInTable functions, is
having those functions be public better than having AtomicString be a friend?

> -	   location = new StringImpl(c, strlen(c), hash); 
> +	   location = StringImpl::create(c).releaseRef(); 
> +	   location->setHash(hash);
> +	   location->setInTable();

Seems slightly unfortunate to set the hash twice and to set the in-table bit
first to 0 and then to 1. Any clean way to avoid that?

On a related note, maybe the best thing here would be to have two create
functions specifically intended for AtomicString's use instead of having the
code here in AtomicString.cpp? Could we then eliminate both setHash and
setInTable? I think that would be good.

> +    void setHash(unsigned hash) {ASSERT(!m_hash); m_hash = hash;}

I don't think it was worthwhile to add this public function that is an error
for anyone except AtomicString to use just so AtomicString doesn't have to be a
friend. Please consider my suggestion above about how to avoid it.

You need some spaces inside the braces here.

review- but this seems almost right!