Attachment 6250 Details for Bug 5210 – patch to fix the problem, including change log and test

[patch] patch to fix the problem, including change log and test

ForInLoopPatch.txt (text/plain), 7.82 KB, created by Darin Adler on 2006-02-04 10:37:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Darin Adler

Created: 2006-02-04 10:37:35 PST

Size: 7.82 KB

patch

obsolete

>Index: JavaScriptCore/kxmlcore/FastMalloc.cpp
>===================================================================
>--- JavaScriptCore/kxmlcore/FastMalloc.cpp	(revision 12552)
>+++ JavaScriptCore/kxmlcore/FastMalloc.cpp	(working copy)
>@@ -95,7 +95,7 @@ void *fastRealloc(void* p, size_t n)
>     return realloc(p, n);
> }
> 
>-#ifndef WIN32
>+#if !WIN32
> void fastMallocRegisterThread(pthread_t thread) 
> {
> }
>@@ -105,32 +105,27 @@ void fastMallocRegisterThread(pthread_t 
> 
> #else
> 
>-#include <new>
>-#include <stdio.h>
>-#include <stddef.h>
>-#if defined HAVE_STDINT_H
>+#if HAVE_STDINT_H
> #include <stdint.h>
>-#elif defined HAVE_INTTYPES_H
>+#elif HAVE_INTTYPES_H
> #include <inttypes.h>
> #else
> #include <sys/types.h>
> #endif
>-#include <string.h>
>-#include <pthread.h>
>-#include <unistd.h>
>-#include <errno.h>
>-#include <stdarg.h>
>-#include "TCSpinLock.h"
>-#include "TCPageMap.h"
>-#include "TCSystemAlloc.h"
> 
>+#include "AlwaysInline.h"
> #include "Assertions.h"
>-
>-#ifdef __GNUC__
>-#define ALWAYS_INLINE inline __attribute__((always_inline))
>-#else
>-#define ALWAYS_INLINE inline
>-#endif
>+#include "TCPageMap.h"
>+#include "TCSpinLock.h"
>+#include "TCSystemAlloc.h"
>+#include <errno.h>
>+#include <new>
>+#include <pthread.h>
>+#include <stdarg.h>
>+#include <stddef.h>
>+#include <stdio.h>
>+#include <string.h>
>+#include <unistd.h>
> 
> #if KXC_CHANGES
> namespace KXMLCore {
>Index: JavaScriptCore/ChangeLog
>===================================================================
>--- JavaScriptCore/ChangeLog	(revision 12555)
>+++ JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,22 @@
>+2006-02-04  Darin Adler  <darin@apple.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        - fix http://bugzilla.opendarwin.org/show_bug.cgi?id=5210
>+          REGRESSION: for/in loop with var changes global variable instead of making local
>+
>+        * kjs/nodes.cpp:
>+        (valueForReadModifyAssignment): Use ALWAYS_INLINE macro.
>+        (ForInNode::execute): Break out of the scope chain loop once we find and set the
>+        loop variable. We don't want to set multiple loop variables.
>+        (ForInNode::processVarDecls): Process the declaration of the loop variable.
>+
>+        - other cleanup
>+
>+        * kjs/object.cpp: (KJS::tryGetAndCallProperty): Use ALWAYS_INLINE macro.
>+        * kxmlcore/FastMalloc.cpp: Change to use ALWAYS_INLINE macro from AlwaysInline.h
>+        instead of defining it here a second time.
>+
> 2006-02-03  Timothy Hatcher  <timothy@apple.com>
> 
>         Reviewed by Justin.
>Index: JavaScriptCore/kjs/object.cpp
>===================================================================
>--- JavaScriptCore/kjs/object.cpp	(revision 12552)
>+++ JavaScriptCore/kjs/object.cpp	(working copy)
>@@ -327,11 +327,7 @@ bool JSObject::deleteProperty(ExecState 
>   return deleteProperty(exec, Identifier::from(propertyName));
> }
> 
>-static inline
>-#ifdef __GNUC__
>-__attribute__((always_inline))
>-#endif
>-JSValue *tryGetAndCallProperty(ExecState *exec, const JSObject *object, const Identifier &propertyName) {
>+static ALWAYS_INLINE JSValue *tryGetAndCallProperty(ExecState *exec, const JSObject *object, const Identifier &propertyName) {
>   JSValue *v = object->get(exec, propertyName);
>   if (v->isObject()) {
>     JSObject *o = static_cast<JSObject*>(v);
>Index: JavaScriptCore/kjs/nodes.cpp
>===================================================================
>--- JavaScriptCore/kjs/nodes.cpp	(revision 12552)
>+++ JavaScriptCore/kjs/nodes.cpp	(working copy)
>@@ -1237,12 +1237,7 @@ JSValue *ConditionalNode::evaluate(ExecS
> 
> // ECMA 11.13
> 
>-#if __GNUC__
>-// gcc refuses to inline this without the always_inline, but inlining it does help
>-static inline JSValue *valueForReadModifyAssignment(ExecState * exec, JSValue *v1, JSValue *v2, Operator oper) __attribute__((always_inline));
>-#endif
>-
>-static inline JSValue *valueForReadModifyAssignment(ExecState * exec, JSValue *v1, JSValue *v2, Operator oper)
>+static ALWAYS_INLINE JSValue *valueForReadModifyAssignment(ExecState * exec, JSValue *v1, JSValue *v2, Operator oper)
> {
>   JSValue *v;
>   int i1;
>@@ -1859,9 +1854,10 @@ Completion ForInNode::execute(ExecState 
>         JSObject *o;
>         do { 
>             o = *iter;
>-            if (o->getPropertySlot(exec, ident, slot))
>+            if (o->getPropertySlot(exec, ident, slot)) {
>                 o->put(exec, ident, str);
>-            
>+                break;
>+            }
>             ++iter;
>         } while (iter != end);
>         
>@@ -1915,6 +1911,8 @@ Completion ForInNode::execute(ExecState 
> 
> void ForInNode::processVarDecls(ExecState *exec)
> {
>+  if (varDecl)
>+    varDecl->processVarDecls(exec);
>   statement->processVarDecls(exec);
> }
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 12556)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,12 @@
>+2006-02-03  Darin Adler  <darin@apple.com>
>+
>+        - test for http://bugzilla.opendarwin.org/show_bug.cgi?id=5210
>+          REGRESSION: for/in loop with var changes global variable instead of making local
>+
>+        * fast/js/for-in-var-scope.html: Added.
>+        * fast/js/resources/for-in-var-scope.js: Added.
>+        * fast/js/for-in-var-scope-expected.txt: Added.
>+
> 2006-02-03  Andrew Wellington  <proton@wiretapped.net>
> 
>         Reviewed by Darin.
>Index: LayoutTests/fast/js/for-in-var-scope-expected.txt
>===================================================================
>--- LayoutTests/fast/js/for-in-var-scope-expected.txt	(revision 0)
>+++ LayoutTests/fast/js/for-in-var-scope-expected.txt	(revision 0)
>@@ -0,0 +1,11 @@
>+This tests that for/in statements properly scope a variable that's declared in one. In previous versions of JavaScriptCore there were two bugs that caused problems. First, the loop variable declaration would not be processed. Second, the code to set the loop variable would incorrectly walk the scope chain even after setting the loop variable.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS i is 'start i'
>+PASS j is 'propName'
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>
>Property changes on: for-in-var-scope-expected.txt
>___________________________________________________________________
>Name: svn:eol-style
>   + native
>
>Index: LayoutTests/fast/js/for-in-var-scope.html
>===================================================================
>--- LayoutTests/fast/js/for-in-var-scope.html	(revision 0)
>+++ LayoutTests/fast/js/for-in-var-scope.html	(revision 0)
>@@ -0,0 +1,13 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<link rel="stylesheet" href="resources/js-test-style.css">
>+<script src="resources/js-test-pre.js"></script>
>+</head>
>+<body>
>+<p id="description"></p>
>+<div id="console"></div>
>+<script src="resources/for-in-var-scope.js"></script>
>+<script src="resources/js-test-post.js"></script>
>+</body>
>+</html>
>
>Property changes on: for-in-var-scope.html
>___________________________________________________________________
>Name: svn:mime-type
>   + text/html
>Name: svn:eol-style
>   + native
>
>Index: LayoutTests/fast/js/resources/for-in-var-scope.js
>===================================================================
>--- LayoutTests/fast/js/resources/for-in-var-scope.js	(revision 0)
>+++ LayoutTests/fast/js/resources/for-in-var-scope.js	(revision 0)
>@@ -0,0 +1,21 @@
>+description(
>+"This tests that for/in statements properly scope a variable that's declared in one. "
>++ "In previous versions of JavaScriptCore there were two bugs that caused problems. "
>++ "First, the loop variable declaration would not be processed. "
>++ "Second, the code to set the loop variable would incorrectly walk the scope chain even after setting the loop variable."
>+);
>+
>+var i = "start i";
>+var j = "start j";
>+
>+function func() {
>+    var object = new Object;
>+    object.propName = "propValue";
>+    for (var i in object) { j = i; }
>+}
>+func();
>+
>+shouldBe("i", "'start i'");
>+shouldBe("j", "'propName'");
>+
>+var successfullyParsed = true;

Flags:

mjs: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 5210: 5883 | 6250