[Webkit-unassigned] [Bug 20422] Patch to allow custom memory allocation control

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20422


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25944|review?                     |review-
               Flag|                            |




------- Comment #42 from darin at apple.com  2009-01-10 15:38 PDT -------
(From update of attachment 25944)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.