Note: There are a few cases of duplicates in user autocompletion which are being worked on.

add BindingIter and use it instead of directly touching a Binding's shape

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 640852 [details] [diff] [review]
patch

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

5 years ago
Created attachment 640853 [details] [diff] [review]
patch

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

5 years ago
Created attachment 640897 [details] [diff] [review]
rm CallObject::setVarOp

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

5 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.
Whiteboard: [js:t]
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 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

5 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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/0be7b0709e5d
https://hg.mozilla.org/mozilla-central/rev/2e8d41e9f8ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.