Bug 121732 – VirtualRegister should be a class

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 212252 [details]
First pass - compiles and runs - need to convert more "int" variables to VirtualRegister

[Michael Saboff]

Created attachment 212469 [details]
Patch - Pretty close to done.  Runs js tests

[Michael Saboff]

Created attachment 212520 [details]
Patch for Review

The style failure already existed.

[Geoffrey Garen]

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.

[Michael Saboff]

Created attachment 212668 [details]
Patch with updates from initial review

[Geoffrey Garen]

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?

[Michael Saboff]

(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.

[Michael Saboff]

Created attachment 212724 [details]
Patch for Landing

[Michael Saboff]

Committed r156474: <http://trac.webkit.org/changeset/156474>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121966

[Michael Saboff]

Committed r156511: <http://trac.webkit.org/changeset/156511>