Thanks for having a look.
-- Vitaly http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#newcode135 src/hydrogen-instructions.h:135: // HLoadFunctionPrototype On 2011/01/26 11:54:00, Kevin Millikin wrote:
These are intended to be alphabetized.
Actually do we need this comment at all? It's a PITA to update it and nothing enforces that what it says is correct. http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#newcode2628 src/hydrogen-instructions.h:2628: SetFlagMask(kDependsOnFunctionPrototypes); On 2011/01/26 11:54:00, Kevin Millikin wrote:
1. Don't introduce a new kDependsOn... flag if there is nothing that specifically kChanges... it (there are only so many bits for these
flags). We
use kDependsOnCalls for this case.
There's DependsOnMaps and no ChangesMaps. We actually discussed this with Florian and decided it's nice to have a more precise dependency in general.
2. SetFlagMask takes a flag mask (something like binary ...00001000,
ie, a
shifted flag) not a flag (something like decimal 3). You want SetFlag
here.
It's error prone and we should change it, but in the meantime we need
to be
careful.
Oops, I totally messed the flags here... http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#newcode2640 src/hydrogen-instructions.h:2640: virtual bool DataEquals(HValue* other) const { return true; } On 2011/01/26 11:54:00, Kevin Millikin wrote:
1. It's not required unless the instruction has kUseGVN. I think you
intend
this one to be GVN-able, so you should set that flag.
Yeah, see above.
2. I think it's a bit error prone to copy the default implementation
instead of
inheriting the default implementation when what you want is the
default
implementation. If we change the default for some reason, this class
would not
see it.
As you and Florian discussed in another thread having it in the base class is actually unsafe, having it here makes it less likely that we'll forget to update it in case the instruction gets more data in the future. http://codereview.chromium.org/6009005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
