please take another look
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 :
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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.
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode50
src/elements.cc:50: // This is an example of Curiously recurring
template pattern
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
Either don't capitalize "Curiously" (and prefix it with a "the"), or
capitalize/title-case all four words.
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode60
src/elements.cc:60: MaybeObject** result) {
The FixedDoubleArray handler (and some of the external array handlers)
allocate heap numbers and can cause a GC, so MaybeObject is correct.
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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.
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
You specialize the only real function, so it's pretty non-identical.
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode180
src/elements.cc:180: inline static bool GetNumberDictionaryElement(
Yes, the NonStrictArgument accessor uses this.
On 2011/08/01 07:33:27, Rico wrote:
Do we use this function outside this class, otherwise just make it
private?
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode217
src/elements.cc:217: return
NumberDictionary::cast(obj->element_dictionary());
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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.
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode264
src/elements.cc:264: class ConcreteElementsHandler : public
ElementsHandler {
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
"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 :)
Fixed. I prefer the classes, because they later will be the home for not
just the GetWithReceiver, but other accessors, too.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode275
src/elements.cc:275: result);
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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?
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.cc#newcode310
src/elements.cc:310: } element_handlers;
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
Seems like a big static initializer. Might upset the Chrome people.
Done.
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#newcode36
src/elements.h:36: // Abstract base class for handles that can operate
on object with differing
On 2011/08/01 07:33:27, Rico wrote:
on object -> an object or objects
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode38
src/elements.h:38: class ElementsHandler {
My plan is to have this class house all of the element-related
operations, so I changed the name to Accessor.
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
What does "Handler" mean. Seems too generic to be meaningful.
http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler*
ForKind(JSObject::ElementsKind elements_kind) {
On 2011/08/01 10:18:23, Kevin Millikin wrote:
No need for 'inline'. Maybe the function name/signature should be
something
like SingletonFor(JSObject::ElementsKind).
Done.
http://codereview.chromium.org/7527001/diff/15002/src/elements.h#newcode48
src/elements.h:48: inline static ElementsHandler*
ForKind(JSObject::ElementsKind elements_kind) {
singleton is not quite right, and I actually like the current naming. in
the code you get something like this:
ElementsAccessor::ForKind(...)
I am not sure that adding Singleton, or ObjectEnum or something of the
like will improve readability. But I'm happy to change it if you feel
strongly.
On 2011/08/01 10:47:13, Lasse Reichstein wrote:
We should probably not call it anything with "Singleton", since that's
a less
general (anti-)pattern (and we don't implement each class as a
Singleton class -
which is a good thing!).
This is more like an object-based enum (q.v. the Java enums, where,
incidentally, "enum Foo" is implemented internally as "class Foo
extends
Enum<Foo>" :)
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();
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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.
Done.
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));
Good question. I think not, since the (header) map + length are 64 bits
together.
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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#newcode74
src/objects.h:74: // - FixedArray
On 2011/08/02 12:23:46, Lasse Reichstein wrote:
I suddently remembered that this documentation is out-of-date now that
there is
a FixedArrayBase. If possible, make it match reality :)
Done.
http://codereview.chromium.org/7527001/diff/15002/src/objects.h#newcode2070
src/objects.h:2070: class FixedArrayBase: public HeapObject {
That's a reasonable observation, but I think I'll leave that for another
CL :-)
On 2011/08/01 09:35:54, Lasse Reichstein wrote:
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
