Closed
Bug 772688
Opened 12 years ago
Closed 12 years ago
add BindingIter and use it instead of directly touching a Binding's shape
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: [js:t])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
In preparation for bug 767013, we need to stop touching the shapes of Bindings directly and instead iterate over some abstract sequence (so that we can stop creating Shapes for unaliased variables).
In the process, I was able to remove the last CallObject::set(Var|Arg)Op JSPropertyOps, which is a nice bonus.
(Note: this patch sits atop bug 753158.)
Attachment #640852 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•12 years ago
|
||
oops, qref'd
Assignee: general → luke
Attachment #640852 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #640852 -
Flags: review?(jwalden+bmo)
Attachment #640853 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•12 years ago
|
||
This patch actually removes set(Var|Arg)Op. (This is based on bug 753158, which is why the barriers shouldn't be necessary.)
Attachment #640897 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•12 years ago
|
||
On a side note, it wouldn't be hard to remove CallObject/StaticBlockObject's use of shortids now. That leaves only the "tinyid" use made in JSAPI. A tinyid is only 8 bits so maybe we could stuff it in the BaseShape::flags? Ideally we'd just remove tinyids, but there seem to be a few browser uses, so I don't know how hard that is.
Updated•12 years ago
|
Whiteboard: [js:t]
Comment 4•12 years ago
|
||
Comment on attachment 640897 [details] [diff] [review]
rm CallObject::setVarOp
Review of attachment 640897 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/methodjit/PolyIC.cpp
@@ -347,5 @@
> - // >--+--< of 'fun' remaining the same. However, since:
> - // ||| 1. the shape includes all arguments and locals and their setters
> - // \\ V and getters, and
> - // \===/ 2. arguments and locals have different getters
> - // then we can rely on fun->nargs remaining invariant.
Oh no, PolyIC.cpp loses its mascot.
Attachment #640897 -
Flags: review?(bhackett1024) → review+
Comment 5•12 years ago
|
||
Comment on attachment 640853 [details] [diff] [review]
patch
Review of attachment 640853 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit-test/tests/arguments/strict-args-flushstack.js
@@ +11,5 @@
> var a = [];
> for (var i = 0; i < 9; i++)
> a.push(arguments);
> + print("args: " + args);
> + print("a[0]: " + a[0]);
Debugging detritus definitely deserves deletion.
::: js/src/jsscript.cpp
@@ +51,5 @@
> using namespace js::gc;
> using namespace js::frontend;
>
> +BindingIter::BindingIter(const Bindings &bindings, Shape *shape)
> + : bindings_(&bindings), shape_(shape)
Needs another space of indentation.
@@ +57,5 @@
> + settle();
> +}
> +
> +BindingIter::BindingIter(Bindings &bindings)
> + : bindings_(&bindings), shape_(bindings.lastBinding)
Another space.
::: js/src/jsscript.h
@@ +25,5 @@
> +struct Shape;
> +
> +namespace mjit {
> + struct JITScript;
> + class CallCompiler;
I haven't seen anyone indenting the contents of namespaces, even for declaring things. I guess this was pre-existing; undo?
@@ +101,5 @@
> +{
> + friend class Bindings;
> + BindingIter(const Bindings &bindings, Shape *shape);
> + void settle();
> + public:
Blank line before this.
Attachment #640853 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (gone July 18-August 26) from comment #5)
> Debugging detritus definitely deserves deletion.
done
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8d41e9f8ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/0be7b0709e5d
Target Milestone: --- → mozilla17
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0be7b0709e5d
https://hg.mozilla.org/mozilla-central/rev/2e8d41e9f8ef
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•