661722 - 'outer' parameter to MethodEnv::newfunction is redundant

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

There's only two call sites, JIT and Interpreter, and both pass the value of env->scope(), so its better to just load it inside newfunction().

Comment 1

[Edwin Smith]

Attachment: Removes 'outer' parameter from MethodEnv::newfunction

Comment 2

[Steven Johnson]

Comment on attachment 537031 [details] [diff] [review]
Removes 'outer' parameter from MethodEnv::newfunction

Review of attachment 537031 [details] [diff] [review]:
-----------------------------------------------------------------

make it so

Comment 3

[Edwin Smith]

Attachment: v2 - also fix a pretty-printing glitch

This updated patch also fixes a logic error in AvmCore::formatOpcode.  i'll carry forward the R+ unless you holler.

Comment 4

[Tamarin Bot]

changeset: 6371:f35b3f3cc133
user:      Edwin Smith <edwsmith>
summary:   Bug 661722 - 'outer' parameter to MethodEnv::newfunction is redundant (r=stejohns+)

http://hg.mozilla.org/tamarin-redux/rev/f35b3f3cc133

Comment 5

[Dan Schaffer]

caused difference when running brightspot -Dverbose=verify against an older vm.  The diff is from newfunction having one fewer parameter e.g.
-  28:newfunction invalid method_id=1566 <anonymous>
+  28:newfunction method_id=1566 <anonymous>

From looking at the change the difference seems expected.  Let me know if it something to be concerned about.

Comment 6

[Steven Johnson]

Attachment: AOT-specific fix, too

AOT uses this call to, have to patch its usage...

Comment 7

[Steven Johnson]

Comment on attachment 537586 [details] [diff] [review]
AOT-specific fix, too

redirecting to Rick since Edwin is on vacation

Comment 8

[Steven Johnson]

Comment on attachment 537586 [details] [diff] [review]
AOT-specific fix, too

pushed (pending review) as 6379:b6046c322d35

Comment 9

[Edwin Smith]

Comment on attachment 537586 [details] [diff] [review]
AOT-specific fix, too

I would have done this had the build system complained.  change looks fine.

Comment 10

[Edwin Smith]

(In reply to comment #5)
> caused difference when running brightspot -Dverbose=verify against an older
> vm.  The diff is from newfunction having one fewer parameter e.g.
> -  28:newfunction invalid method_id=1566 <anonymous>
> +  28:newfunction method_id=1566 <anonymous>
> 
> From looking at the change the difference seems expected.  Let me know if it
> something to be concerned about.

Expected change, no problem.