Bug 101127

Created attachment 172179 [details] Patch

Attachment 172179 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2386: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2412: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2425: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172179 [details] Patch Attachment 172179 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14724024

Comment on attachment 172179 [details] Patch Attachment 172179 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14721047

Created attachment 172184 [details] Patch

Attachment 172184 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2386: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2412: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2425: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172184 [details] Patch Attachment 172184 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14725023

Comment on attachment 172184 [details] Patch Attachment 172184 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14718081

Comment on attachment 172184 [details] Patch Attachment 172184 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14697856

Comment on attachment 172184 [details] Patch Attachment 172184 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14725073

Comment on attachment 172184 [details] Patch Attachment 172184 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14725077 New failing tests: fast/js/basic-strict-mode.html fast/js/mozilla/strict/assign-to-callee-name.html

*sigh* One day all C++ compilers will support the same language.

Comment on attachment 172184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172184&action=review > Source/JavaScriptCore/ChangeLog:13 > + This patch adds a marginally more compact form of bytecode that is > + free from any data specific to a given execution context, and that > + does store any data structures necessary for execution. To actually This sentence is confusing, I think you meant "does*n't* store any data structures necessary for execution". > Source/JavaScriptCore/API/tests/testapi.c:1463 > - v = JSObjectCallAsFunction(context, function, NULL, 0, NULL, NULL); > + exception = 0; > + v = JSObjectCallAsFunction(context, function, NULL, 0, NULL, &exception); > + ASSERT(!exception); Is this change related? > Source/JavaScriptCore/bytecode/CodeBlock.cpp:674 > - dataLog("[%4d] new_array_buffer %s, %d, %d", location, registerName(exec, dst).data(), argv, argc); > + dataLog("[%4d] new_array_buffer\t %s, %d, %d", location, registerName(exec, dst).data(), argv, argc); This change seems unrelated. > Source/JavaScriptCore/bytecode/CodeBlock.h:872 > } > + > + > unsigned addOrFindConstant(JSValue); Why so much new whitespace? > Source/JavaScriptCore/bytecode/CodeBlock.h:1357 > + // TODO: This could just be a pointer to m_unlinkedCodeBlock's data, but the DFG mutates > + // it, so we're stuck with it for now. We usually label these FIXME: rather than TODO. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:269 > +void UnlinkedProgramCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor) > +{ > + UnlinkedProgramCodeBlock* thisObject = jsCast<UnlinkedProgramCodeBlock*>(cell); > + ASSERT_GC_OBJECT_INHERITS(thisObject, &s_info); > + COMPILE_ASSERT(StructureFlags & OverridesVisitChildren, OverridesVisitChildrenWithoutSettingFlag); > + ASSERT(thisObject->structure()->typeInfo().overridesVisitChildren()); > + Base::visitChildren(thisObject, visitor); > + for (size_t i = 0, end = thisObject->m_functionDeclarations.size(); i != end; i++) > + visitor.append(&thisObject->m_functionDeclarations[i].second); > +} > + This would read a bit more cleanly if there was some whitespace paragraphing. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:55 > +class Debugger; > +class FunctionBodyNode; > +class FunctionExecutable; > +class FunctionParameters; > +struct ParserError; > +class ScriptExecutable; > +class SourceCode; > +class SourceProvider; > +class SharedSymbolTable; > +class UnlinkedCodeBlock; > +class UnlinkedFunctionCodeBlock; The struct should be at the end. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:76 > +class UnlinkedFunctionExecutable : public JSCell { > +public: This class should probably have its own file. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:102 > + String paramString() const; I don't think there is a great reason to shorten parameter to param here, especially since we use the full word elsewhere. I also don't think the name makes it to clear what this function does. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:143 > + unsigned m_numCapturedVariables : 29; > + bool m_forceUsesArguments : 1; > + bool m_isInStrictContext : 1; > + bool m_hasCapturedVariables : 1; Can these be packed with CodeFeatures below to be smaller on 64bit? > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:193 > + } > + > +}; This newline is extraneous. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:495 > + int m_numParameters; > + JSGlobalData* m_globalData; > + > + int m_thisRegister; > + int m_argumentsRegister; > + int m_activationRegister; > + > + bool m_needsFullScopeChain : 1; > + bool m_usesEval : 1; > + bool m_isNumericCompareFunction : 1; > + bool m_isStrictMode : 1; > + bool m_isConstructor : 1; > + bool m_hasCapturedVariables : 1; > + unsigned m_firstLine; > + unsigned m_lineCount; > + > + CodeFeatures m_features; > + CodeType m_codeType; Is this optimally packed? > Source/JavaScriptCore/heap/MarkedAllocator.h:81 > +#ifndef NDEBUG > + memset(result, 0xCD, bytes); > +#endif This could use a comment. > Source/JavaScriptCore/heap/MarkedAllocator.h:88 > +#ifndef NDEBUG > + memset(head, 0xCD, bytes); > +#endif As could this. > Source/JavaScriptCore/runtime/CodeCache.h:52 > +class EvalExecutable; > +class Identifier; > +class ProgramExecutable; > +class UnlinkedCodeBlock; > +class UnlinkedEvalCodeBlock; > +class UnlinkedFunctionCodeBlock; > +class UnlinkedFunctionExecutable; > +class UnlinkedProgramCodeBlock; > +class JSGlobalData; > +struct ParserError; > +class SourceCode; > +class SourceProvider; These should be sorted with the struct at the end.

Created attachment 172449 [details] Patch

Attachment 172449 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2387: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2413: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2426: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172449 [details] Patch Attachment 172449 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14714900

Comment on attachment 172449 [details] Patch Attachment 172449 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14745236

Qt folk: can you fix your toolchain? enum Foo { Bar, Wibble } int foo(Foo f) { switch (f) { case Bar: return 1; case Wibble: return 2; } } should not warn about not returning a value, and yet that is what it is doing

Created attachment 172461 [details] Patch

Attachment 172461 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2387: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2413: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2426: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172461 [details] Patch Attachment 172461 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14755012

Comment on attachment 172461 [details] Patch Attachment 172461 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14682301

(In reply to comment #21) > (From update of attachment 172461 [details]) > Attachment 172461 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/14755012 Dammit, what are the qt project files again?

Created attachment 172627 [details] Patch

Attachment 172627 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2408: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2421: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 51 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172627 [details] Patch Attachment 172627 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14733814

Created attachment 172646 [details] Patch

Attachment 172646 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2408: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2421: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 3 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 172646 [details] Patch Attachment 172646 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14757142

Comment on attachment 172646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172646&action=review R=me! :-D > Source/JavaScriptCore/heap/Heap.cpp:497 > } > } > #endif > - > + > if (m_globalData->codeBlocksBeingCompiled.size()) { > GCPHASE(VisitActiveCodeBlock); > for (size_t i = 0; i < m_globalData->codeBlocksBeingCompiled.size(); i++) > m_globalData->codeBlocksBeingCompiled[i]->visitAggregate(visitor); > } > - > + > { > GCPHASE(VisitMachineRoots); > MARK_LOG_ROOT(visitor, "C++ Stack"); Kill! > Source/JavaScriptCore/jit/JIT.cpp:339 > + case op_init_global_const_nop: > + NEXT_OPCODE(op_init_global_const_nop); When would this survive into the JIT?

Committed r133688: <http://trac.webkit.org/changeset/133688>

Comment on attachment 172646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172646&action=review > Source/JavaScriptCore/runtime/CodeCache.cpp:39 > + : m_randomGenerator(static_cast<uint32_t>(randomNumber() * UINT32_MAX)) I believe this expression generates a random number in the range 0..0xFFFFFFFE and never generates 0xFFFFFFFF.