Bug 150720

Created attachment 264411 [details] work in progress

Created attachment 264413 [details] the patch

Attachment 264413 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:540: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:541: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:541: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:573: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:574: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:574: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:603: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:604: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:604: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:67: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirCode.cpp:88: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3Value.cpp:146: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3Value.cpp:178: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:47: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:48: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 15 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

View in context: https://bugs.webkit.org/attachment.cgi?id=264413&action=review > Source/JavaScriptCore/b3/B3Const64Value.cpp:68 > + return proc.add<Const64Value>(origin(), m_value == other->asInt64()); Oops. > Source/JavaScriptCore/b3/B3Const64Value.cpp:75 > + return proc.add<Const64Value>(origin(), m_value != other->asInt64()); ditto > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:68 > + return proc.add<ConstDoubleValue>(origin(), m_value == other->asDouble()); ditto > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:75 > + return proc.add<ConstDoubleValue>(origin(), m_value != other->asDouble()); ditto > Source/JavaScriptCore/b3/B3LowerToAir.cpp:683 > + // In B3 it's possible to branch on any value. The semantics of: > + // > + // Branch(value) > + // > + // Are guaranteed to be identical to: > + // > + // Branch(NotEqual(value, 0)) I have the feeling this comment would have more visibility next to the opcode definition instead of at the lowering phase. > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:253 > + m_changed = true; > + m_changedCFG = true; Might be easier to loop over (m_changed || m_changedCFG) since m_changedCFG implies m_changed > Source/JavaScriptCore/b3/B3UpsilonValue.h:42 > + void setPhi(Value* phi) { m_phi = phi; } ASSERT(phi->type() == [value]->type()); ASSERT(phi->opcode() == phi) > Source/JavaScriptCore/b3/B3UpsilonValue.h:57 > + if (phi) ASSERT(phi->opcode() == phi)

Comment on attachment 264413 [details] the patch Please check the previous comments before landing.

(In reply to comment #4) > View in context: > https://bugs.webkit.org/attachment.cgi?id=264413&action=review > > > Source/JavaScriptCore/b3/B3Const64Value.cpp:68 > > + return proc.add<Const64Value>(origin(), m_value == other->asInt64()); > > Oops. > > > Source/JavaScriptCore/b3/B3Const64Value.cpp:75 > > + return proc.add<Const64Value>(origin(), m_value != other->asInt64()); > > ditto > > > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:68 > > + return proc.add<ConstDoubleValue>(origin(), m_value == other->asDouble()); > > ditto > > > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:75 > > + return proc.add<ConstDoubleValue>(origin(), m_value != other->asDouble()); > > ditto Fixed those. > > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:683 > > + // In B3 it's possible to branch on any value. The semantics of: > > + // > > + // Branch(value) > > + // > > + // Are guaranteed to be identical to: > > + // > > + // Branch(NotEqual(value, 0)) > > I have the feeling this comment would have more visibility next to the > opcode definition instead of at the lowering phase. B3Opcode.h has a sort of similar comment. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:253 > > + m_changed = true; > > + m_changedCFG = true; > > Might be easier to loop over (m_changed || m_changedCFG) since m_changedCFG > implies m_changed I changed it so that the "if (m_changedCFG)" check in the fixpoint also sets m_changed, so if you set m_changedCFG it implies m_changed automatically. > > > Source/JavaScriptCore/b3/B3UpsilonValue.h:42 > > + void setPhi(Value* phi) { m_phi = phi; } > > ASSERT(phi->type() == [value]->type()); > ASSERT(phi->opcode() == phi) > > > Source/JavaScriptCore/b3/B3UpsilonValue.h:57 > > + if (phi) > > ASSERT(phi->opcode() == phi) Fixed.

Landed in http://trac.webkit.org/changeset/191816