Re: [PATCH] binary heap implementation

Assert(!"description of error") is an idiom I've seen more than once,
although I'm not sure I understand why it's not written as Robert says with
the condition in the brackets (or as a print to STDERR followed by abort()
instead).


On 15 November 2012 15:11, Robert Haas <robertmhaas@gmail.com> wrote:

> 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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>