Bug 23114 - JSValue* should be replaced with a class type.
Summary: JSValue* should be replaced with a class type.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-04 21:17 PST by Gavin Barraclough
Modified: 2009-01-06 21:12 PST (History)
0 users

See Also:


Attachments
The patch (898.54 KB, patch)
2009-01-04 21:18 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
With added 'doze jsc build fix goodness (898.75 KB, patch)
2009-01-05 14:58 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
revert emitLoad function name change & fix typo in ChangeLog. (897.25 KB, patch)
2009-01-05 17:06 PST, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff
patch relative to 603, to test on windows [tot was broken earlier.] (897.26 KB, patch)
2009-01-06 16:30 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2009-01-04 21:17:30 PST
Presently the representation of JavaScript values varies depending on the platform, (e.g. on x86-64 we can represent 32-bit integers in JSValue*s, but on x86 we can only represent 31-bit integers).  Furthermore we cannot override casting operators to control conversions – which, for example, means that noValue() must be 0 (since the code relies on being able to ask "if (someJSValue) {" meaning "if (someJSValue != noValue()) {", and also means we cannot control casting between pointer types, making it problematic to store pointers in anything but their canonical form.

Encapsulating JSValue*s in a smart pointer type will allow us greater flexibility in and control over the internal representation.
Comment 1 Gavin Barraclough 2009-01-04 21:18:19 PST
Created attachment 26422 [details]
The patch

Testing on windows before setting review flag.
Comment 2 Gavin Barraclough 2009-01-05 14:58:58 PST
Created attachment 26443 [details]
With added 'doze jsc build fix goodness
Comment 3 Gavin Barraclough 2009-01-05 17:06:51 PST
Created attachment 26447 [details]
revert emitLoad function name change & fix typo in ChangeLog.

revert emitLoad name change & fix type in ChangeLog.
Comment 4 Oliver Hunt 2009-01-06 12:00:51 PST
Comment on attachment 26447 [details]
revert emitLoad function name change & fix typo in ChangeLog.

> +        value now encapsulaed it will likely make sense to migrate the functionality
tyop

Looks good, am only rubber stamping the rename portion of it, r=me on the JSValuePtr (and EncodedAsPtr) conversion logic, assuming you've tested jit + interpreter code paths, and perf is good
Comment 5 Gavin Barraclough 2009-01-06 16:30:55 PST
Created attachment 26480 [details]
patch relative to 603, to test on windows [tot was broken earlier.]
Comment 6 Gavin Barraclough 2009-01-06 21:12:34 PST
Transmitting file data .........................................................................................................................................................................................................................................................................................................
Committed revision 39670.