Re: [PATCH] binary heap implementation

On Wed, Nov 14, 2012 at 8:11 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> Comments? Suggestions?

It could use a run through pgindent.  And the header comments are
separated by a blank line from the functions to which they are
attached, which is not project style.

+    if (heap->size + 1 == heap->space)
+        Assert("binary heap is full");

This doesn't really make any sense.  Assert("binary heap is full")
will never fail, because that expression is a character string which
is, therefore, not NULL.  I think you want Assert(heap->size <
heap->space).  Or you could use (heap->size + 1 >= heap->space)
elog(LEVEL, message) if there's some reason this needs to be checked
in non-debug builds.

+    {
+        sift_down(heap, i);
+    }

Project style is to omit braces for a single-line body.  This comes up
a few other places as well.

Other than the coding style issues, I think this looks fine.  If you
can fix that up, I believe I could go ahead and commit this, unless
anyone else objects.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company