Bug 56214

Summary: Ensure all values are correctly tagged in the registerfile
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, buildbot, bweinstein, commit-queue, eric, rniwa, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 56251    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Oliver Hunt 2011-03-11 12:52:30 PST 
Ensure all values are correctly tagged in the registerfile
Comment 1 Oliver Hunt 2011-03-11 13:07:17 PST 
Created attachment 85516 [details]
Patch
Comment 2 Early Warning System Bot 2011-03-11 13:27:34 PST 
Attachment 85516 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8132369
Comment 3 Oliver Hunt 2011-03-11 13:34:36 PST 
Created attachment 85520 [details]
Patch
Comment 4 Build Bot 2011-03-11 13:50:22 PST 
Attachment 85516 [details] did not build on win:
Build output: http://queues.webkit.org/results/8132382
Comment 5 Gavin Barraclough 2011-03-11 14:09:06 PST 
Comment on attachment 85520 [details]
Patch

Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj change should probably be reverted?

> *this = JSValue(reinterpret_cast<JSCell*>(activation));

These casts are all incorrect, they should be static_casts (reinterpret_cast is almost always the wrong cast when casting within a class hierarchy).
But can't you just delete all these overloaded= operators? - why doesn't the compiler just call the operator=(JSValue) automatically?

If we're going to access the two halves of the value like we do in JSVALUE32_64 it seems like we should have the IntValueDescriptor defined in JSValue, where it is for JSVALUE32_64 (better still, unify the two!)

r- for the reinterpreting.
Comment 6 Build Bot 2011-03-11 15:00:29 PST 
Attachment 85520 [details] did not build on win:
Build output: http://queues.webkit.org/results/8132411
Comment 7 Oliver Hunt 2011-03-11 15:09:47 PST 
Created attachment 85543 [details]
Patch
Comment 8 Early Warning System Bot 2011-03-11 15:24:20 PST 
Attachment 85543 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8135475
Comment 9 Build Bot 2011-03-11 16:03:50 PST 
Attachment 85543 [details] did not build on win:
Build output: http://queues.webkit.org/results/8133485
Comment 10 Oliver Hunt 2011-03-11 16:09:20 PST 
Created attachment 85551 [details]
Patch
Comment 11 Gavin Barraclough 2011-03-11 16:30:28 PST 
Comment on attachment 85551 [details]
Patch

r is me
Comment 12 Build Bot 2011-03-11 19:01:02 PST 
Attachment 85551 [details] did not build on win:
Build output: http://queues.webkit.org/results/8149539
Comment 13 WebKit Commit Bot 2011-03-11 19:12:25 PST 
Comment on attachment 85551 [details]
Patch

Clearing flags on attachment: 85551

Committed r80919: <http://trac.webkit.org/changeset/80919>
Comment 14 WebKit Commit Bot 2011-03-11 19:12:29 PST 
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2011-03-11 19:38:32 PST 
http://trac.webkit.org/changeset/80919 might have broken Windows Release (Build) and Windows Debug (Build)
Comment 16 Ryosuke Niwa 2011-03-11 23:08:51 PST 
It seems like this change caused a Windows build failure:
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/13160
Comment 17 Oliver Hunt 2011-03-12 11:42:41 PST 
The commit queue is broken and needs to be disabled.  I'll re-land on monday with the windows fix.
Comment 18 Adam Barth 2011-03-12 11:54:16 PST 
> The commit queue is broken and needs to be disabled.

https://bugs.webkit.org/show_bug.cgi?id=56251#c5
Comment 19 Oliver Hunt 2011-03-14 11:21:58 PDT 
Committed r81040: <http://trac.webkit.org/changeset/81040>
Comment 20 WebKit Review Bot 2011-03-14 11:29:19 PDT 
http://trac.webkit.org/changeset/81040 might have broken Windows Release (Build) and Windows Debug (Build)