On 2012-11-21 15:09:12 -0500, Robert Haas wrote: > On Wed, Nov 21, 2012 at 1:30 PM, Alvaro Herrera > <alvhe...@2ndquadrant.com> wrote: > > The following comments still talk about "key and value", thus need > > an update: > > Oops. > > > This comment needs updated (s/comparator/compare/, and also add > > has_heap_property and arg): > > Fixed. > > > I would suggest to add prefixes to struct members, so bh_size, bh_space > > and so on. This makes it easier to grep for stuff later. > > OK. > > > Do we really need the struct definition be public? Couldn't it just > > live in binaryheap.c? > > Well, that would preclude defining any macros, like > binaryheap_empty(). Since efficiency is among the reasons for > inventing this in the first place, that doesn't seem prudent to me. > > Another new version attached.
One very minor nitpick I unfortunately just found now, not sure when that changed: binaryheap_replace_first() hardcodes the indices for the left/right node of the root node. I would rather have it use (left|right)_offset(0). > P.S. to Abhijit: Please, please tell me the secret to getting five > people to review your patches. I'm lucky when I can get one! Heh. This was rather surprising, yes. Its a rather easy patch to review in a way, you don't need much pg internals knowledge for it... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
