This change is fishy (sorry I didn't see it before). I will fix it.
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 These are intended to be alphabetized. http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#newcode225 src/hydrogen-instructions.h:225: V(LoadFunctionPrototype) \ So are these. http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#newcode2628 src/hydrogen-instructions.h:2628: SetFlagMask(kDependsOnFunctionPrototypes); 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. 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. 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; } 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. 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. http://codereview.chromium.org/6009005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
