[webkit-reviews] review denied: [Bug 20422] Patch to allow custom memory allocation control : [Attachment 25944] JavaScriptCore FastMalloc-related patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Paul Pedriana <ppedriana at gmail.com>'s
request for review:
Bug 20422: Patch to allow custom memory allocation control
https://bugs.webkit.org/show_bug.cgi?id=20422

Attachment 25944: JavaScriptCore FastMalloc-related patch.
https://bugs.webkit.org/attachment.cgi?id=25944&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The work looks good and about ready to land.

> +	       fastFree(p);  // We don't need to check for a null pointer; the
compiler does this.

The comment is curious, because fastFree also checks for a null pointer. I'm
not really sure what we're saying with the comment.

> +	   void* p = fastMalloc(sizeof(T));
> +
> +	   if (p) {
> +	       fastMallocMatchValidateMalloc(p, Internal::AllocTypeFastNew);
> +	       return ::new(p) T;
> +	   }
> +
> +	   return 0;

In general in this project, we prefer the "early return" style, so we'd check
for null and return 0, then put the success case code in a straight line rather
than nested in an if statement.

> +    if(static_cast<size_t>(~0) - sizeof(AllocAlignmentInteger) <= n)  // If
overflow would occur...

We put a space after the if and before the "(" character.

It's OK to use static_cast<size_t>(~0), but it doesn't seem better than
numeric_limits<size_t>::max() to me.

> +    n += sizeof(AllocAlignmentInteger);
> +    char* result = static_cast<char*>(malloc(n));

I think it's slightly better to write this as:

    char* result = static_cast<char*>(malloc(sizeof(AllocAlignmentInteger) +
n));

rather than modifying the argument. On the minus side, it makes the line of
code a little more complex, but on the plus side it avoids modifying an
argument which can sometimes be confusing. I'm also not sure that making the
result be a char* is all that valuable, since we have to cast it with
reinterpret_cast. If we made it a void* we could use static_cast instead.

> +	   *reinterpret_cast<AllocAlignmentInteger*>(result) =
Internal::AllocTypeMalloc;
> +	   result += sizeof(AllocAlignmentInteger);

With void* this would be:

    *static_cast<AllocAlignmentInteger*>(result) = Internal::AllocTypeMalloc;
    result = static_cast<AllocAlignmentInteger*>(result) + 1;

Or:

    AllocAlignmentInteger* header =
static_cast<AllocAlignmentInteger*>(result);
    *header = Internal::AllocTypeMalloc;
    result = header + 1;

> +	   AllocAlignmentInteger* pValue =
Internal::fastMallocMatchValidationValue(p);
> +	   if (*pValue != Internal::AllocTypeMalloc)
> +	       Internal::fastMallocMatchFailed(p);
> +	   free(pValue);

We almost never use the "Hungarian" style where a varible is named "pType",
which you're using here. I'd personally prefer a letter or a word, maybe
"header".

> +    new_ptr = static_cast<char*>(new_ptr) + sizeof(AllocAlignmentInteger);

I don't see a good reason to involve char* here. I'd use this:

    new_ptr = static_cast<AllocAlignmentInteger*>(new_ptr) + 1;

> +	       const AllocAlignmentInteger* pType = reinterpret_cast<const
AllocAlignmentInteger*>(static_cast<const char*>(p) -
sizeof(AllocAlignmentInteger));

Again, unnecessary use of char* and reinterpret_cast here.

    const AllocAlignmentInteger* pType = static_cast<const
AllocAlignmentInteger*>(p) - 1;


> +    // These could alternatively be inline functions which do nothing.
> +    #define fastMallocMatchValidateMalloc(p, type)
> +    #define fastMallocMatchValidateFree(p, type)

And they should be. We'd prefer empty inline functions to macros for this sort
of thing.

>  /* ENABLE macro defaults */
> +#if !defined(ENABLE_FAST_MALLOC_MATCH_VALIDATION)
> +#define ENABLE_FAST_MALLOC_MATCH_VALIDATION 0
> +#endif

Our platform header is really getting out of hand. A long list of things with
no documentation about what they are! Not that your single addition makes it
significantly worse.

I think there were just enough comments that I'll say review-, although I think
this is in great shape and nearly ready to go. I don't think there's any one
comment above that's important, but I'm hoping you'll want to take at least one
of my suggestions above.