596068 - The kIsScriptEnv bit is clumsy

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Steven Johnson]

The AIR runtime requires the existence of ScriptObject::isGlobalObject; we currently use the kIsScriptEnv bit on MethodEnv/ScriptEnv to implement this... which works, but is awkward and makes for extra bit-twiddling in the already-overloaded activationOrMCTable mess in MethodEnv.

A much simpler approach would be to make isGlobalObject() a virtual function (defaulting to false), and make a new subclass of ScriptObject (used only when we allocate the ScriptEnv's global field) that returns true.

Unlikely to be any meaningful net gain or loss in either code or performance, but resulting code should be cleaner IMHO.

(Note that there is already an acceptance test: acceptance/misc/isGlobalObject.abc)

Comment 1

[Steven Johnson]

Attachment: Patch

Comment 2

[Edwin Smith]

A bit on Traits would also do it, without using native subclassing; the bit could be set right in AbcParser.

Comment 3

[Steven Johnson]

Better yet, we can just check for Traits::posType() == TRAITSTYPE_SCRIPT

Comment 4

[Steven Johnson]

Attachment: Patch v2

Comment 5

[Edwin Smith]

Comment on attachment 475573 [details] [diff] [review]
Patch v2

Much cleaner, R+ with nits below addressed, one way or another.

MethodEnv::kIsScriptEnv looks dead now.  remove?

A few carefully placed asserts could check that ScriptObject::isGlobalObject() returns the correct value.  I think we only allocate global objects in one place, and that place should return true, all other allocation sites.  Other testing ideas welcome.

Comment 6

[Steven Johnson]

(In reply to comment #5)
> MethodEnv::kIsScriptEnv looks dead now.  remove?

Oops. Yeah.

Comment 7

[Steven Johnson]

pushed with nits: http://hg.mozilla.org/tamarin-redux/rev/9f9d08440c3e