Summary: | Make JSValue bool conversion less dangerous | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||
Component: | New Bugs | Assignee: | Oliver Hunt <oliver> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, benjamin, cmarcelo, commit-queue, darin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Oliver Hunt
2013-08-29 19:51:12 PDT
Created attachment 210060 [details]
Patch
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.
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! 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. (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 :( (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. Created attachment 210130 [details]
Patch
> > 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. 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. Committed r154902: <http://trac.webkit.org/changeset/154902> |