Summary: | VirtualRegister should be a class | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Michael Saboff
2013-09-20 20:48:35 PDT
Created attachment 212252 [details]
First pass - compiles and runs - need to convert more "int" variables to VirtualRegister
Created attachment 212469 [details]
Patch - Pretty close to done. Runs js tests
Created attachment 212520 [details]
Patch for Review
The style failure already existed.
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. Created attachment 212668 [details]
Patch with updates from initial review
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? (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. Created attachment 212724 [details]
Patch for Landing
Committed r156474: <http://trac.webkit.org/changeset/156474> Re-opened since this is blocked by bug 121966 Committed r156511: <http://trac.webkit.org/changeset/156511> |