[webkit-reviews] review denied: [Bug 41177] References for movable objects : [Attachment 62960] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nathan Lawrence
<nlawrence at apple.com>'s request for review:
Bug 41177: References for movable objects
https://bugs.webkit.org/show_bug.cgi?id=41177

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * bytecode/CodeBlock.cpp:
> +	   (JSC::CodeBlock::markAggregate):
> +	   * runtime/Collector.cpp:
> +	   (JSC::Heap::markConservatively):
> +	   * runtime/JSCell.h:
> +	   (JSC::JSValue::asCell):
> +	   (JSC::MarkStack::append):
> +	   (JSC::MarkStack::appendInternal):
> +	   * runtime/JSGlobalObject.cpp:
> +	   (JSC::markIfNeeded):
> +	   * runtime/JSONObject.cpp:
> +	   (JSC::Stringifier::Holder::object):
> +	   * runtime/JSObject.h:
> +	   (JSC::JSObject::prototype):
> +	   * runtime/JSStaticScopeObject.cpp:
> +	   (JSC::JSStaticScopeObject::markChildren):
> +	   * runtime/JSValue.h:
> +	   (JSC::JSValue::):
> +	   (JSC::JSValue::JSValue):
> +	   (JSC::JSValue::asCell):
> +	   * runtime/MarkStack.h:
> +	   * runtime/NativeErrorConstructor.cpp:
> +	   (JSC::NativeErrorConstructor::createStructure):
> +	   (JSC::NativeErrorConstructor::markChildren):
> +	   * runtime/NativeErrorConstructor.h:
> +	   * runtime/Structure.h:
> +	   (JSC::Structure::storedPrototype):

If you're not going to write function by function comments, which is OK, I
suggest you remove stray bogus function names that prepare-ChangeLog puts in.
For example "(JSC::JSValue::)". I personally prefer to include function by
function comments. A useful way to think through your patch one extra time
while writing a brief explanation of each change.

> -		   markStack.append(reinterpret_cast<JSCell*>(xAsBits));
> +		   JSCell* cell = reinterpret_cast<JSCell*>(xAsBits);
> +		   markStack.append(cell);

It's unclear why this is OK. Normally we need to call append on references in
place, not temporary copies on the stack. I think a comment here is needed to
explain why it's OK to do it this way here.

> +	   friend class CollectorHeap;

Adding friends is OK, but always unfortunate. If there is a way to avoid it,
that would be better.

> +	   /* References in C++ are not covariant, so we fudge around it with
> +	    * this nonsense.
> +	    */

A couple thoughts about this comment:

    1) We use C-style "//" comments rather than "/*" comments in almost all
cases. You should here.

    2) While it's fine to use an informal tone "fudge around it with this
nonsense" is probably too vague for some readers. If you say something clearer
then you have a chance to explain why this code is a good idea. For example,
here is something helpful, but probably a bit too long: "That rule prevents
writing a pointer of a base class type into a more-specifically typed pointer.
But here, the only changes we will make would be to update the pointer to a
moved copy of the same object, so there is no danger of changing the type. We
use a typecast to bypass the normal rule." Calling the idiom "nonsense" will
create unnecessary fear, uncertainty, and doubt for in future readers of the
code.

> +    ALWAYS_INLINE void MarkStack::append(Register& reg)

Please don't use a non-word abbreviation when there's a word available. I guess
you're avoiding "register" because it's a C++ keyword? Is there another word
you can use here?

> -static inline void markIfNeeded(MarkStack& markStack, JSValue v)
> +template<typename T>
> +static void markIfNeeded(MarkStack& markStack, T*& v)

I think this would read better on a single line rather than two lines.

> -static inline void markIfNeeded(MarkStack& markStack, const
RefPtr<Structure>& s)
> +static void markIfNeeded(MarkStack& markStack, RefPtr<Structure>& s)

Why did you remove the inline keyword here? Per-function comments in the change
log are a great place to explain decisions like this one.

> +	   template<typename T>
> +	   ALWAYS_INLINE void append(T*& cell);
> +	   ALWAYS_INLINE void append(JSValue&);
> +	   ALWAYS_INLINE void append(Register&);

Here the function template declaration would be much easier to read if it was
on one line rather than two!

> +void NativeErrorConstructor::markChildren(MarkStack& markStack)
> +{
> +    InternalFunction::markChildren(markStack);
> +    // HACKY_FIX:
> +    // The prototype pointer here needs to be updated
> +    //
> +    // Not sure if there's a better solution
> +    markStack.append(m_errorStructure->storedPrototype());
> +}

I don't think "HACKY_FIX" here is appropriate. If you think there's something
wrong here, you need to say what. This code looks just fine to me, so there
must be something specific about it that seems wrong to you. You should say
what it is and why it's wrong in as specific a manner as possible to give later
programmers a chance.

>      private:
>	   virtual ConstructType getConstructData(ConstructData&);
>	   virtual CallType getCallData(CallData&);
>  
> +	   virtual void markChildren(MarkStack&);
> +
>	   virtual const ClassInfo* classInfo() const { return &info; }
>  
>	   RefPtr<Structure> m_errorStructure;
> +
> +    protected:
> +	   static const unsigned StructureFlags =
InternalFunction::StructureFlags | OverridesMarkChildren;

Normally we would put a protected member like this above the private members,
unless there's a good reason not to.

> +	   JSValue const &storedPrototype() const { return m_prototype; }
> +	   JSValue &storedPrototype() { return m_prototype; }

The "&" characters should be next to the type name and const, not the member
name.

This is really close to a review+, but I'll say review- so you can fix one or
more of the things I mentioned above.