[webkit-reviews] review granted: [Bug 24739] Combine StringImpl struct and data allocations : [Attachment 28828] updated patch to address comments.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 23 08:53:42 PDT 2009


Darin Adler <darin at apple.com> has granted Mike Belshe <mike at belshe.com>'s
request for review:
Bug 24739: Combine StringImpl struct and data allocations
https://bugs.webkit.org/show_bug.cgi?id=24739

Attachment 28828: updated patch to address comments.
https://bugs.webkit.org/attachment.cgi?id=28828&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +// Some of the factory methods create buffers using fastMalloc.
> +// We must ensure that all allocations of StringImpl are allocated using
> +// fastMalloc so that we don't have mis-matched frees.   We accomplish 
> +// this by overriding the new and delete operators.

We use one space after a period, not three.

> +void *StringImpl::operator new(size_t bytes, void *mem)

We use void*, not void *.

The name "bytes" seems wrong for the argument, since the argument is a number
of bytes, not actual "bytes". Perhaps the name "size" or "sizeInBytes".

I'm not sure why you chose to abbreviate "memory" to "mem", but please don't.

> +void *StringImpl::operator new(size_t bytes)

Again, void*.

> +    // Allocate a single buffer large enough to contain the StringImpl
> +    // struct as well as the data which it contains.  This removes one 
> +    // heap allocation from this call.

One space, not two, after a period.

> +    int size = sizeof(StringImpl) + (length * sizeof(UChar));

size_t, not int. Also, I don't think the parentheses make things clearer and I
would omit them.

> +    // Allocate a single buffer large enough to contain the StringImpl
> +    // struct as well as the data which it contains.  This removes one 
> +    // heap allocation from this call.

One space, not two.

> +    int size = sizeof(StringImpl) + (length * sizeof(UChar));

size_t, not int. Also, I don't think the parentheses make things clearer and I
would omit them.

> +    void *operator new(size_t bytes);

void*, not void *. Same comment as above about the argument name "bytes".

> +    // Allocation from a custom buffer is only allowed internally to avoid
> +    // mismatched allocators.
> +    void *operator new(size_t bytes, void *mem);

This comment seems a little strange. All allocation is only allowed internally.
Can't you make all the operator new declarations private?

void*, not void *. Same comment as above about the argument name "bytes".

> +    // In some cases, we allocate the StringImpl struct and its data
> +    // within a single heap buffer.	In this case, the m_data pointer
> +    // is an "internal buffer", and does not need to be deallocated.

One space, not two, after a period.

r=me as is -- the comments above are all minor nitpicks


More information about the webkit-reviews mailing list