Bug 146154 – Employ explicit operator bool() instead of using the UnspecifiedBoolType workaround.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This also fixes a bug in CodeOrigin::isApproximatelyEqualTo() due to an unexpected implicit cast from the UnspecifiedBoolType workaround.

[Mark Lam]

Created attachment 255209 [details]
the patch.

[Darin Adler]

Comment on attachment 255209 [details]
the patch.

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

I’m a little surprised that adopting the real operator bool involves introducing so many cases of !!, which I have always thought of as an idiom we wanted to avoid, popular on Windows for some reason.

> Source/JavaScriptCore/bytecode/CodeOrigin.cpp:77
> +        if (a.inlineCallFrame->executable.get() != b.inlineCallFrame->executable.get())

Looks like this is possibly a change in behavior. Is this a bug fix?

I prefer that we add an overload for the != operator rather than adding the calls to .get() here.

> Source/JavaScriptCore/bytecode/LLIntCallLinkInfo.h:48
> +    bool isLinked() { return !!callee; }

Ugly. Any better alternative? Maybe a local variable?

> Source/JavaScriptCore/heap/Handle.h:55
> +    explicit operator bool() const { return (m_slot && *m_slot); }

No need for the parentheses here.

> Source/JavaScriptCore/runtime/JSArray.cpp:379
> +            bool hadValue = !!valueSlot;

I think you can write this modern variant:

    bool hadValue { valueSlot };

I, for one, like that better than !!.

[Filip Pizlo]

Comment on attachment 255209 [details]
the patch.

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

>> Source/JavaScriptCore/bytecode/CodeOrigin.cpp:77
>> +        if (a.inlineCallFrame->executable.get() != b.inlineCallFrame->executable.get())
> 
> Looks like this is possibly a change in behavior. Is this a bug fix?
> 
> I prefer that we add an overload for the != operator rather than adding the calls to .get() here.

I agree with this change.

I believe that isApproximatelyEqualTo() is not used on any critical path - it's mostly for our bytecode profiler.  Mark, can you confirm?

[Mark Lam]

Comment on attachment 255209 [details]
the patch.

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

>>> Source/JavaScriptCore/bytecode/CodeOrigin.cpp:77
>>> +        if (a.inlineCallFrame->executable.get() != b.inlineCallFrame->executable.get())
>> 
>> Looks like this is possibly a change in behavior. Is this a bug fix?
>> 
>> I prefer that we add an overload for the != operator rather than adding the calls to .get() here.
> 
> I agree with this change.
> 
> I believe that isApproximatelyEqualTo() is not used on any critical path - it's mostly for our bytecode profiler.  Mark, can you confirm?

To answer Darin's question, yes, this is also a bug fix.

To answer Fil, this is used in CallLinkStatus::ContextMap which in turn is used for poly-variant call inlining, ByteCodeParser::handleCall(), and ByteCodeParser::handleVarargsCall().  It doesn't look related to the bytecode profiler.

>> Source/JavaScriptCore/bytecode/LLIntCallLinkInfo.h:48
>> +    bool isLinked() { return !!callee; }
> 
> Ugly. Any better alternative? Maybe a local variable?

I'm opting to leave this as is as it is more concise.

>> Source/JavaScriptCore/runtime/JSArray.cpp:379
>> +            bool hadValue = !!valueSlot;
> 
> I think you can write this modern variant:
> 
>     bool hadValue { valueSlot };
> 
> I, for one, like that better than !!.

Fixed.

[Mark Lam]

Thanks for the review.  Landed in r185768: <http://trac.webkit.org/r185768>.