The kIsScriptEnv bit is clumsy

RESOLVED FIXED

Status

Tamarin
Virtual Machine
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.27 KB, patch
Edwin Smith
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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)
(Assignee)

Comment 1

8 years ago
Created attachment 474896 [details] [diff] [review]
Patch
Attachment #474896 - Flags: review?(edwsmith)

Comment 2

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

Updated

8 years ago
Attachment #474896 - Flags: review?(edwsmith) → review-
(Assignee)

Comment 3

8 years ago
Better yet, we can just check for Traits::posType() == TRAITSTYPE_SCRIPT
(Assignee)

Comment 4

8 years ago
Created attachment 475573 [details] [diff] [review]
Patch v2
Attachment #474896 - Attachment is obsolete: true
Attachment #475573 - Flags: review?(edwsmith)

Comment 5

8 years ago
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.
Attachment #475573 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 6

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

Oops. Yeah.
(Assignee)

Comment 7

8 years ago
pushed with nits: http://hg.mozilla.org/tamarin-redux/rev/9f9d08440c3e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.