[webkit-reviews] review denied: [Bug 45059] Add red-black tree capable of holding plain old data (POD) : [Attachment 66391] Revised patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 45059: Add red-black tree capable of holding plain old data (POD)
https://bugs.webkit.org/show_bug.cgi?id=45059

Attachment 66391: Revised patch
https://bugs.webkit.org/attachment.cgi?id=66391&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66391&action=prettypatch

> WebCore/platform/graphics/gpu/PODArena.h:41
> +#include <wtf/Vector.h>
perhaps you should explicitly include <stdint.h> since you use uint8_t below.

> WebCore/platform/graphics/gpu/PODArena.h:56
> +	   virtual ~Allocator() { }
nit: though it may not be that common in WebCore, I find it helpful
when destructors on classes that inherit from RefCounted are made
protected.  To do that you need a friend declaration:

  friend class WTF::RefCounted<Allocator>;

This way no one else can delete the object.  Only the RefCounted
base class can delete the object.

(Ditto for PODArena itself.)

> WebCore/platform/graphics/gpu/PODArena.h:84
> +    static PassRefPtr<PODArena> create(Allocator* allocator)
nit: this should take a PassRefPtr<Allocator> argument

> WebCore/platform/graphics/gpu/PODArena.h:90
> +    template<class T>
nit: on member functions, it seems more common in webkit code to
put the template keyword on the same line as the function name:

  template<class T> T* alloc()

nit: perhaps allocateObject would be a better name for this method.

> WebCore/platform/graphics/gpu/PODArena.h:141
> +    static size_t minAlignment()
wtf/Vector.h defines a macro named WTF_ALIGN_OF which can be used to
determine the alignment requirements for a type using a compiler
specific mechanism.

perhaps minAlignment() could be rewritten like so:

  static size_t minAlignment()
  {
      return WTF_ALIGN_OF(T);
  }

> WebCore/platform/graphics/gpu/PODArena.h:153
> +    RefPtr<Allocator> m_allocator;
nit: i think it is best to put all of the member variables together.
so, i would put this member variable below the Chunk definition.

> WebCore/platform/graphics/gpu/PODArena.h:200
> +    size_t m_currentChunkSize;
why have m_currentChunkSize and not just a size() member function
on the Chunk class?  (if you want to keep m_currentChunkSize, then
perhaps it should at least be listed next to m_current since it is
so closely related.)

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:53
> +//	String valueToString(const T&);
nit: valueToString should probably be a const method

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:79
> +#ifndef NDEBUG
nit: I'd just make a separate section below the main set of includes
for the debug-only includes.  that way there will be fewer #ifndefs.

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:121
> +    explicit PODRedBlackTree(PODArena* arena)
nit: use PassRefPtr<PODArena> here

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:150
> +    bool contains(const T& data) { return treeSearch(data); }
it seems like you could slap a const on contains, visitInorder and size

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:189
> +    void setVerboseDebugging(bool onOrOff)
nit: normally, in webkit a parameter name like this is named like the
method, so:  void setVerboseDebugging(bool verboseDebugging)

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:197
> +	   kRed = 1,
nit: enum values should be named with the "k" prefix

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:446
> +	       LOG_ERROR("  PODRedBlackTree::InsertNode");
why LOG_ERROR?	wouldn't LOG(PODRedBlackTree, "InsertNode") be better?
you can see an example of using LOG this way in RefCountedLeakCounter.cpp

also how about writing an inline helper function that can have an empty
implementation in NDEBUG builds but otherwise call LOG(...).  that way
the code in this function will not need #ifdefs.

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:688
> +	   Counter()
nit: no line break here

> WebCore/platform/graphics/gpu/PODRedBlackTree.h:750
> +	   LOG_ERROR("%s", builder.toString().ascii().data());
ditto about using LOG instead

I know there was some discussion before about the use of x, y, z, and
w variable names, but I didn't find them to be a problem.  I'm not sure
that calling them "node" or "newNode" or "tempNode" would really help
improve readability at all.