[webkit-reviews] review denied: [Bug 64466] Refactor JSC to replace JSCell::operator new with static create method : [Attachment 100695] Proposed patch.

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 64466: Refactor JSC to replace JSCell::operator new with static create
method
https://bugs.webkit.org/show_bug.cgi?id=64466

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

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100695&action=review


In general this patch is correct, there are just a few overall refinements i'd
recommend

* You don't need to clear the structure everytime, that's only needed as a
workaround for the existing sequencing issues.
* Given you don't need to clear the structure a single function along the lines
of template <typename T> void* allocateCell<T>(exec/globalData/heap/whatever)
can just be used for all allocations, and the create methods become 
return new (allocateCell<type goes here>(exec/globalData/whatever))
Constructor(...)

That should simplify the creation routines quite a bit.

If you use the webkit-patch script it will run the style checker before
uploading so you can correct any style issues before submitting for review (the
commit queue is meant to reject patches with style errors so theoretically
wouldn't accept a patch with such errors)

> Source/JavaScriptCore/ChangeLog:6
> +

There needs to be a description of the change here, look for other changelog
entries for examples of the level of detail that's expected.  While this is
mostly a mechanical change, so doesn't require huge amounts of elaboration, we
typically expect a reasonable amount of detail from new-ish contributors.

> Source/JavaScriptCore/API/JSCallbackConstructor.h:41
> +	   result->m_structure.clear();

You don't need these calls to clear the structure -- they exist in operator
new() to mitigate the problem that this patch is fixing.

> Source/JavaScriptCore/API/JSCallbackObject.h:136
> +    static JSCallbackObject* allocCallbackObj(ExecState* exec)
> +    {
> +	   JSCallbackObject* result =
static_cast<JSCallbackObject*>(exec->heap()->allocate(sizeof(JSCallbackObject))
);
> +	   result->m_structure.clear();
> +	   return result;
> +    }
> +public:
> +    static JSCallbackObject* create(ExecState* exec, JSGlobalObject*
globalObject, Structure* structure, JSClassRef classRef, void* data)
> +    {
> +	   return new (allocCallbackObj(exec)) JSCallbackObject(exec,
globalObject, structure, classRef, data);
> +    }
> +    static JSCallbackObject* create(JSGlobalData* globalData, JSClassRef
classRef, Structure* structure)
> +    {
> +	   return new (globalData) JSCallbackObject(*globalData, classRef,
structure);
> +    }

i think allocCallbackObj can be private, and you should kill off the new
(globalData) usage

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:585
> +    JSArray* resObj = JSArray::create(exec, exec->globalData(),
exec->lexicalGlobalObject()->arrayStructure(), deleteCount, CreateCompact);

The additional exec shouldn't be necessary.

> Source/JavaScriptCore/runtime/BooleanObject.h:33
> +	   static BooleanObject* create(ExecState* exec, JSGlobalData&
globalData, Structure* structure)

You don't need the exec parameter -- just use globalData.heap (exec->heap() is
actually doing exec->globalData().heap)

> Source/JavaScriptCore/runtime/DateInstance.h:37
> +	   static DateInstance* allocDateInstance(ExecState* exec)

This can probably be private.

> Source/JavaScriptCore/runtime/Error.cpp:172
> +    : InternalFunction(&exec->globalData(), globalObject, structure,
exec->globalData().propertyNames->emptyIdentifier)
> +    , m_message(message)

should be indented

> Source/JavaScriptCore/runtime/Executable.h:327
> +	       ProgramExecutable* result =
static_cast<ProgramExecutable*>(exec->heap()->allocate(sizeof(ProgramExecutable
)));

The more i look at this pattern the more i wonder if we want to add a void*
allocate<T>() { return allocate(sizeof(T)); } style function to heap.

Also these casts are unnecessary, void* is fine (as you don't need to clear the
structure)

> Source/JavaScriptCore/runtime/GetterSetter.h:43
> +	   : JSCell(exec->globalData(),
exec->globalData().getterSetterStructure.get())
> +	   {
> +	   }

indentation

> Source/JavaScriptCore/runtime/JSCell.h:89
>      protected:
>	   enum VPtrStealingHackType { VPtrStealingHack };
> -
> +	   
>      private:
>	   explicit JSCell(VPtrStealingHackType) { }
>	   JSCell(JSGlobalData&, Structure*);
>	   JSCell(JSGlobalData&, Structure*, CreatingEarlyCellTag);
>	   virtual ~JSCell();
>	   static const ClassInfo s_dummyCellInfo;
> -
> +	   
> +	   

Random whitespace changes == badness

> Source/JavaScriptCore/runtime/JSCell.h:-168
> +	   WriteBarrier<Structure> m_structure;
>  
>      private:
>	   // Base implementation; for non-object classes implements
getPropertySlot.
>	   virtual bool getOwnPropertySlot(ExecState*, const Identifier&
propertyName, PropertySlot&);
>	   virtual bool getOwnPropertySlot(ExecState*, unsigned propertyName,
PropertySlot&);
>	   
> -	   WriteBarrier<Structure> m_structure;

This should be unnecessary once you remove the extraneous calls to clear()