[Webkit-unassigned] [Bug 100566] JSC: a JSC printf (support for %J+s and %b)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=100566


Michael Saboff <msaboff at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #171028|review?                     |review-
               Flag|                            |




--- Comment #2 from Michael Saboff <msaboff at apple.com>  2012-10-30 10:48:15 PST ---
(From update of attachment 171028)
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'.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.