Bug 120505 – Make JSValue bool conversion less dangerous

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Make JSValue bool conversion less dangerous

[Oliver Hunt]

Created attachment 210060 [details]
Patch

[WebKit Commit Bot]

Attachment 210060 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/CommonSlowPaths.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h', u'Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp', u'Source/JavaScriptCore/runtime/PropertyDescriptor.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:186:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:187:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 210060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210060&action=review

> Source/JavaScriptCore/jit/JITStubs.cpp:348
> +        if (UNLIKELY(static_cast<bool>(stackFrame.vm->exception()))) \

Why do you need these casts?

> Source/JavaScriptCore/runtime/JSCJSValue.h:177
> +    typedef void* (JSValue::*UnspecifiedBoolType);
> +    operator UnspecifiedBoolType*() const;

Of course, if you were using C++11 you could just declare operator bool() as explicit!

[Darin Adler]

Comment on attachment 210060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210060&action=review

> Source/JavaScriptCore/interpreter/CallFrame.h:78
> +        bool hadException() const { return static_cast<bool>(vm().exception()); }

Maybe !! is better than static_cast<bool>?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:223
> +        if (UNLIKELY(static_cast<bool>(callFrame->vm().exception())))

I’m surprised you need all these casts inside UNLIKELY. Seems like a bug in the UNLIKELY macro perhaps.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:216
> +    return (tag() != EmptyValueTag) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;

No need for the parentheses here.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:363
> +    return (u.asInt64) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;

No need for the parentheses here.

> Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:188
> -    if (!other.m_value == m_value ||
> -        !other.m_getter == m_getter ||
> -        !other.m_setter == m_setter)
> +    if (!other.m_value == static_cast<bool>(m_value) ||
> +        !other.m_getter == static_cast<bool>(m_getter) ||
> +        !other.m_setter == static_cast<bool>(m_setter))

Can this bizarre code possibly be what's intended? I really don’t understand what's going on here. I also think this would read better on a single line.

[Oliver Hunt]

(In reply to comment #3)
> (From update of attachment 210060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210060&action=review
> 
> > Source/JavaScriptCore/jit/JITStubs.cpp:348
> > +        if (UNLIKELY(static_cast<bool>(stackFrame.vm->exception()))) \
> 
> Why do you need these casts?

UNLIKELY turns into builtin expect, which expects a long, and won't try to coerce

> 
> > Source/JavaScriptCore/runtime/JSCJSValue.h:177
> > +    typedef void* (JSValue::*UnspecifiedBoolType);
> > +    operator UnspecifiedBoolType*() const;
> 
> Of course, if you were using C++11 you could just declare operator bool() as explicit!

I have given up working out what features are okay, and what are not :(

[Oliver Hunt]

(In reply to comment #4)
> (From update of attachment 210060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210060&action=review
> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:78
> > +        bool hadException() const { return static_cast<bool>(vm().exception()); }
> 
> Maybe !! is better than static_cast<bool>?

i'll see if there's an isEmpty() function and do !exception.isEmpty() is that reasonable?

> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:223
> > +        if (UNLIKELY(static_cast<bool>(callFrame->vm().exception())))
> 
> I’m surprised you need all these casts inside UNLIKELY. Seems like a bug in the UNLIKELY macro perhaps.

Hmm, that does seem like a better fix

> 
> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:216
> > +    return (tag() != EmptyValueTag) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> 
> No need for the parentheses here.
But i like parenthesis (maybe i just want RPN++? :D )

> 
> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:363
> > +    return (u.asInt64) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> 
> No need for the parentheses here.
O_o

Yeah, that's a bit excessive

> 
> > Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:188
> > -    if (!other.m_value == m_value ||
> > -        !other.m_getter == m_getter ||
> > -        !other.m_setter == m_setter)
> > +    if (!other.m_value == static_cast<bool>(m_value) ||
> > +        !other.m_getter == static_cast<bool>(m_getter) ||
> > +        !other.m_setter == static_cast<bool>(m_setter))
> 
> Can this bizarre code possibly be what's intended? I really don’t understand what's going on here. I also think this would read better on a single line.

Oh yeah, it's intended, it took me a while to work out what it was doing (and i think i wrote this code)

Anyway this patch is still ugly, i just wanted to run it through the build systems to find any other issues.

[Oliver Hunt]

Created attachment 210130 [details]
Patch

[Alexey Proskuryakov]

> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:216
> > +    return (tag() != EmptyValueTag) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> 
> No need for the parentheses here.

This is actually something I ask people to do. Less thinking required to read the code, and makes it look more like other conditions (if or while).

> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:363
> > +    return (u.asInt64) ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> 
> No need for the parentheses here.

Yet I agree that there should be no parentheses in trivial conditions without spaces.

[Darin Adler]

Comment on attachment 210130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210130&action=review

> Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:188
> +    if (other.m_value.isEmpty() != m_value.isEmpty()
> +        || other.m_getter.isEmpty() != m_getter.isEmpty()
> +        || other.m_setter.isEmpty() != m_setter.isEmpty())

Much easier to read! Still would like to see this merged onto a single line.

> Source/WTF/wtf/Compiler.h:226
> +#define UNLIKELY(x) __builtin_expect(static_cast<bool>(x), 0)

Is !! better than the cast? Do we want this cast even in the non-GCC side of the macro?

> Source/WTF/wtf/Compiler.h:237
> +#define LIKELY(x) __builtin_expect(static_cast<bool>(x), 1)

Ditto.

[Oliver Hunt]

Committed r154902: <http://trac.webkit.org/changeset/154902>