http://codereview.chromium.org/7527001/diff/15002/src/elements.cc File src/elements.cc (right):
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode44 src/elements.cc:44: // class SomeElementsHandlerImpl : I really, really dislike adding "Impl" to a class name. Any non-purely-virtual class is an implementation, so "Impl" is vacuous. Personally, I use "ElementsHandlerBase" for the base class, and just "SomeElementsHandler" for the subclass, but that's not really optimal either. http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode50 src/elements.cc:50: // This is an example of Curiously recurring template pattern Either don't capitalize "Curiously" (and prefix it with a "the"), or capitalize/title-case all four words. http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode60 src/elements.cc:60: MaybeObject** result) { Why MaybeObject as result, if you always return an Object? http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode99 src/elements.cc:99: // identical except for specialization. You specialize the only real function, so it's pretty non-identical. http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode217 src/elements.cc:217: return NumberDictionary::cast(obj->element_dictionary()); Why specialize this? The element_dictionary method does exactly the same as the generic GetBackingStore function, and the extra cast to NumberDictionary here isn't needed, since element_dictionary returns a NumberDictionary already. http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode264 src/elements.cc:264: class ConcreteElementsHandler : public ElementsHandler { "Concrete" sounds like a weasel-word too. And it's not a word I would use for a template :) If ElementsHandler is abstract, call it AbstractElementsHandler (except that it's bad naming, because it makes no sense for an is-a relationship, so call it something else). Being concrete should be the default. I.e., a class structure something like: class ElementsGetter { public: virtual bool GetWithReceiver(JSObject* obj, ...) = 0; static ElementsGetter* ForKind(JSObject::ElementsKind elements_kind); private: ElementsGetter* getters[]; } template <typename GetterType, typename BackingStoreType> class TemplatedElementsGetter: public ElementsGetter { public: virtual bool GetWithReceiver(...) { ... } private: BackingStoreType* GetBackingStore(JSObject* obj) { ... } ... } class PixelElementsGetter : public TemplatedElementsGetter<PixelElementsGetter, ExternalPixelArray> { }; As a different idea, maybe just make it an array of functions that you can call directly, instead of an array of objects that you call virtual functions on. Should save an indirection. (Yes, I'm a functional programmer by heart :) http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode275 src/elements.cc:275: result); Why is this split into ElementsHandler and ElementsHandlerImpl at all? Can't you just have your handler struct contain the ElementsHandlerImpl subclasses directly, and let those GetWithReceiver functions be virtual. I.e. remove the classes you call ElementsHandler and ConcreteElementsHandler completely? http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode310 src/elements.cc:310: } element_handlers; Seems like a big static initializer. Might upset the Chrome people. http://codereview.chromium.org/7527001/diff/15002/src/elements.h File src/elements.h (right): http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode38 src/elements.h:38: class ElementsHandler { What does "Handler" mean. Seems too generic to be meaningful. http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1604 src/objects-inl.h:1604: return get(index)->IsTheHole(); Is this really used so much that it needs this shorthand? I would have thought that in the cases where you need the check, you also need the value afterwards, if it isn't the hole. http://codereview.chromium.org/7527001/diff/15002/src/objects-inl.h#newcode1680 src/objects-inl.h:1680: return is_the_hole_nan(READ_DOUBLE_FIELD(this, offset)); Probably stupid question, but can this read be unaligned? http://codereview.chromium.org/7527001/diff/15002/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode2070 src/objects.h:2070: class FixedArrayBase: public HeapObject { Preferably this should be called FixedArray, as the generic superclass of all fixed-size arrays, and the original FixedArray should be ObjectFixedArray. I can see why it wasn't done, though, having done the same thing myself elsewhere :) http://codereview.chromium.org/7527001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
