Bug 121732

Summary: VirtualRegister should be a class
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, mark.lam, mhahnenberg, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 121966    
Bug Blocks: 116888    
Attachments:
Description Flags
First pass - compiles and runs - need to convert more "int" variables to VirtualRegister
none
Patch - Pretty close to done. Runs js tests
none
Patch for Review
none
Patch with updates from initial review
ggaren: review+
Patch for Landing none

Description Michael Saboff 2013-09-20 20:48:35 PDT
VirtualRegister is currently an enum, but this offer little type safety and breaks when a VirtualRegister variable is used for an array index.

Instead it should be a POD class with a single int member.
Comment 1 Michael Saboff 2013-09-20 20:52:09 PDT
Created attachment 212252 [details]
First pass - compiles and runs - need to convert more "int" variables to VirtualRegister
Comment 2 Michael Saboff 2013-09-24 08:25:43 PDT
Created attachment 212469 [details]
Patch - Pretty close to done.  Runs js tests
Comment 3 Michael Saboff 2013-09-24 16:41:38 PDT
Created attachment 212520 [details]
Patch for Review

The style failure already existed.
Comment 4 Geoffrey Garen 2013-09-24 17:36:34 PDT
Comment on attachment 212520 [details]
Patch for Review

View in context: https://bugs.webkit.org/attachment.cgi?id=212520&action=review

> Source/JavaScriptCore/ChangeLog:18
> +        * bytecode/CodeBlock.cpp:
> +        (JSC::CodeBlock::registerName):

I'm not a fan of this long, unannotated list.

> Source/JavaScriptCore/bytecode/VirtualRegister.h:38
> +// enum VirtualRegister { InvalidVirtualRegister = 0x3fffffff };

Please remove this commented-out code.

> Source/JavaScriptCore/bytecode/VirtualRegister.h:50
> +    VirtualRegister(int virtualRegister)

This should be explicit, so we don't automatically convert unknown data into this typed format. Otherwise, we don't get any type safety.

> Source/JavaScriptCore/bytecode/VirtualRegister.h:65
> +    VirtualRegister operator+(int offset) { return VirtualRegister(m_virtualRegister + offset); }

This is another way to accidentally convert an int to a VirtualRegister without explicitly specifying your intent, so let's get rid of it.

> Source/JavaScriptCore/bytecode/VirtualRegister.h:89
> +    operator int() { return m_virtualRegister; }

This is a hole in our type safety, since it allows someone to accidentally cast a VirtualRegister into an index into an array of locals or arguments. Let's remove it.
Comment 5 Michael Saboff 2013-09-25 22:30:15 PDT
Created attachment 212668 [details]
Patch with updates from initial review
Comment 6 Geoffrey Garen 2013-09-25 23:34:37 PDT
Comment on attachment 212668 [details]
Patch with updates from initial review

View in context: https://bugs.webkit.org/attachment.cgi?id=212668&action=review

> Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:27
> +#ifndef DFGAssemblyHelpers_h
> +#define DFGAssemblyHelpers_h

What's up with this file move?
Comment 7 Michael Saboff 2013-09-26 07:04:07 PDT
(In reply to comment #6)
> (From update of attachment 212668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212668&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:27
> > +#ifndef DFGAssemblyHelpers_h
> > +#define DFGAssemblyHelpers_h
> 
> What's up with this file move?

I think it got moved while I was working on this patch.  I'll rebase my patch so that I don't move it back.
Comment 8 Michael Saboff 2013-09-26 10:48:23 PDT
Created attachment 212724 [details]
Patch for Landing
Comment 9 Michael Saboff 2013-09-26 10:49:37 PDT
Committed r156474: <http://trac.webkit.org/changeset/156474>
Comment 10 WebKit Commit Bot 2013-09-26 11:13:28 PDT
Re-opened since this is blocked by bug 121966
Comment 11 Michael Saboff 2013-09-26 15:52:48 PDT
Committed r156511: <http://trac.webkit.org/changeset/156511>