[webkit-reviews] review granted: [Bug 26249] Support JSON.stringify : [Attachment 31039] First pass at JSON.stringify

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 26249: Support JSON.stringify
https://bugs.webkit.org/show_bug.cgi?id=26249

Attachment 31039: First pass at JSON.stringify
https://bugs.webkit.org/attachment.cgi?id=31039&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
The code looks good, but I think it could use a little sprucing up in the way
of whitespace/paragraphing.  If you would, please go through and add some
whitespace to make it more in line with the rest of the code.  A few specifics
inline.

> +
> +class Stringifier {
> +    typedef UString StringBuilder;
> +    // Basically the maximum nesting accepted by firefox.

Perhaps given the complexity of this Stringifier class, we should put it in its
own header file.

> +    }
> +    class StringKeyGenerator {
> +    public:
> +	   StringKeyGenerator(const Identifier& property) :
m_property(property) {}
> +	   JSValue getKey(ExecState* exec) { if (!m_key) m_key = jsString(exec,
m_property.ustring()); return m_key;  }
> +    private:
> +	   Identifier m_property;
> +	   JSValue m_key;
> +    };
> +    class IntKeyGenerator {
> +    public:
> +	   IntKeyGenerator(uint32_t property) : m_property(property) {}
> +	   JSValue getKey(ExecState* exec) { if (!m_key) m_key = jsNumber(exec,
m_property); return m_key;  }
> +    private:
> +	   uint32_t m_property;
> +	   JSValue m_key;
> +    };

A little whitespace in and around these classes could help paragraph them a
little better.

> +	       return jsValue;
> +
> +	   JSValue list[] = { jsKey.getKey(m_exec) };	     
> +	   ArgList args(list, sizeof(list) / sizeof(JSValue));
> +	   return call(m_exec, object,callType, callData, jsValue, args);
Missing space after object,.


> +	   return true;
> +    }
> +    bool stringifyObject(StringBuilder& builder, JSObject* object) {

{ on the wrong line.  Please put whitespace between functions as well.

> +
> +// ECMA-262 v5 15.12.3
> +JSValue JSC_HOST_CALL JSONProtoFuncStringify(ExecState* exec, JSObject*,
JSValue thisValue, const ArgList& args)
> +{
> +    UNUSED_PARAM(thisValue);

I believe you can just elide the name thisValue in the prototype and avoid the
need for this UNUSED_PARAM

r=me.