The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hello,
Adding some review comments:
1. In list_member_ptr, will it be okay to bring `const ListCell *cell` from
#ifdef USE_NO_SIMD
const ListCell *cell;
#endif
to #else like as mentioned below? This will make visual separation between #if cases more cleaner
#else
const ListCell *cell;
foreach(cell, list)
{
if (lfirst(cell) == datum)
return true;
}
return false;
#endif
2. Lots of duplicated
if (list == NIL) checks before calling list_member_inline_internal(list, datum)
Can we not add this check in list_member_inline_internal itself?
3. if (cell)
return list_delete_cell(list, cell) in list_delete_int/oid
can we change if (cell) to if (cell != NULL) as used elsewhere?
4. list_member_inline_interal_idx , there is typo in name.
5. list_member_inline_interal_idx and list_member_inline_internal are practically same with almost 90+ % duplication.
can we not refactor both into one and allow cell or true/false as return? Although I see usage of const ListCell so
thismight be problematic.
6. Loop for (i = 0; i < tail_idx; i += nelem_per_iteration) for Vector32 in list.c looks duplicated from pg_lfind32 (in
pg_lfind.h),can we not reuse that?
7. Is it possible to add a benchmark which shows improvement against HEAD ?
Regards,
Ankit
The new status of this patch is: Waiting on Author
