On Mon, Oct 31, 2022 at 6:58 PM Aleksander Alekseev <aleksan...@timescale.com> wrote: > > Hi hackers, > > > I will take another look at v3 tomorrow and probably mark it RfC. > > I very much like the patch. While on it: > > ``` > +static inline bool > +dclist_is_empty(dclist_head *head) > +{ > + Assert(dlist_is_empty(&head->dlist) == (head->count == 0)); > + return (head->count == 0); > +} > ``` > > Should we consider const'ifying the arguments of the dlist_*/dclist_* > functions that don't change the arguments?
+1, but as a separate discussion/thread/patch IMO. > Additionally it doesn't seem that we have any unit tests for dlist / > dclist. Should we consider adding unit tests for them to > src/test/regress? Most of the dlist_* functions are being covered I guess. AFAICS, dclist_* functions that aren't covered are dclist_insert_after(), dclist_insert_before(), dclist_pop_head_node(), dclist_move_head(), dclist_move_tail(), dclist_has_next(), dclist_has_prev(), dclist_next_node(), dclist_prev_node(), dclist_head_element_off(), dclist_head_node(), dclist_tail_element_off(), dclist_head_element(). IMO, adding an extension under src/test/modules to cover missing or all dlist_* and dclist_* functions makes sense. It improves the code coverage. FWIW, test_lfind is one such recent test extension. > To clarify, IMO both questions are out of scope of this specific patch > and should be submitted separately. You're right, both of them must be discussed separately. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com