[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