Re: [PATCH] binary heap implementation - Mailing list pgsql-hackers

From Robert Haas
Subject Re: [PATCH] binary heap implementation
Date
Msg-id CA+TgmoZb2P3=gy+qtpR02GfdsUfJUkDpv2=wz=GF+42oeTerzw@mail.gmail.com
Whole thread Raw
In response to [PATCH] binary heap implementation  (Abhijit Menon-Sen <ams@2ndQuadrant.com>)
Responses Re: [PATCH] binary heap implementation  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: [PATCH] binary heap implementation  (Will Crawford <billcrawford1970@gmail.com>)
Re: [PATCH] binary heap implementation  (Andrew Dunstan <andrew@dunslane.net>)
Re: [PATCH] binary heap implementation  (Abhijit Menon-Sen <ams@2ndQuadrant.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: [v9.3] Extra Daemons (Re: elegant and effective way for running jobs inside a database)
Next
From: Andres Freund
Date:
Subject: Re: [PATCH 02/14] Add support for a generic wal reading facility dubbed XLogReader