[webkit-reviews] review denied: [Bug 100566] JSC: a JSC printf (support for %J+s and %b) : [Attachment 171028] the patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 30 10:46:56 PDT 2012


Michael Saboff <msaboff at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 100566: JSC: a JSC printf (support for %J+s and %b)
https://bugs.webkit.org/show_bug.cgi?id=100566

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

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=171028&action=review


What about tests?

Overall it looks good, but I'd like to see these comments addressed.

> Source/JavaScriptCore/ChangeLog:13
> +	   - %J+s prints the WTF::String in verbose mode.
> +	     The + means verbose.

Consider switching the order of these two lines.  '+' is a modifier and %J+s is
an example of it's use.

> Source/JavaScriptCore/ChangeLog:16
> +	   (JSC):

The prepare-ChangeLog script put this in but messed up what it is for.	Change
it to the right item or remove.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:112
> +

Remove, only need one blank line between declarations.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:139
> +

Remove this empty line.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:142
> +

Remove.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:154
> +    char buffer[128];

Seems like the buffer might be too small.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:227
> +	       // verbose mode prints: WTF::String "<your string>"

This comment isn't needed here since you have the same comment above
printWTFString.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:237
> +	       p--;

This is not good enough.  Consider the handling of the unsupported %J+z.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:251
> +	   char* argFormat = buffer;

Do you need argFormat or could you just use buffer?  Using buffer could
eliminate future confusion that buffer is available for use between here and
the end of the while loop.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:254
> +	   if (UNLIKELY(curr >= end))
> +	       goto formatTooLong;

Don't think the overflow check is needed here, since curr was just set to
buffer.

Also, you could create a buffer overflow macro instead of putting the if ...
goto everywhere.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:263
> +	   if (c == 'l' || c == 'h') {

What about hh, ll, L and the more obscure j, t and z length modifiers?

> Source/JavaScriptCore/interpreter/VMInspector.cpp:275
> +	       ASSERT(curr[-1] == '\0');

Seems a little unnecessary given the assignment and if.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:295
> +    p -= 5;

Why subtract 5?  Is this to move before the current format?  Why not just use
format for the error message?

> Source/JavaScriptCore/interpreter/VMInspector.cpp:302
> +    // We've got an error. Can't do any more work. Print and error message
if

Should be "an" instead of "and".

> Source/JavaScriptCore/interpreter/VMInspector.cpp:357
> +	       const LChar* chars = str->characters8();
> +	       printArg("%s", reinterpret_cast<const char*>(chars));

Don't think you need the temp.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:360
> +	       const UChar* chars = str->characters16();
> +	       printArg("%S", reinterpret_cast<const wchar_t*>(chars));

Ditto.

> Source/JavaScriptCore/interpreter/VMInspector.cpp:445
> +	   int count = ::vsnprintf(m_buffer, m_size, format, args);

I don't think it matters too much, but count doesn't include the terminating
'\0'.


More information about the webkit-reviews mailing list