On Sat, Sep 23, 2017 at 5:56 AM, Sokolov Yura <funny.fal...@postgrespro.ru> wrote: > Hello, Claudio. > > Thank you for review and confirm of improvement. > > > On 2017-09-23 01:12, Claudio Freire wrote: >> >> >> Patch 1 applies cleanly, builds, and make check runs fine. >> >> The code looks similar in style to surrounding code too, so I'm not >> going to complain about the abundance of underscores in the macros :-p >> >> I can reproduce the results in the OP's benchmark, with slightly >> different numbers, but an overall improvement of ~6%, which matches >> the OP's relative improvement. >> >> Algorithmically, everything looks sound. >> >> >> A few minor comments about patch 1: >> >> + if (max == 1) >> + goto end; >> >> That goto is unnecessary, you could just as simply say >> >> if (max > 1) >> { >> ... >> } > > > Done. > (I don't like indentation, though :-( ) > >> >> >> +#define pg_shell_sort_pass(elem_t, cmp, off) \ >> + do { \ >> + int _i, _j; \ >> + elem_t _temp; \ >> + for (_i = off; _i < _n; _i += off) \ >> + { \ >> >> _n right there isn't declared in the macro, and it isn't an argument >> either. It should be an argument, having stuff inherited from the >> enclosing context like that is confusing. >> >> Same with _arr, btw. > > > pg_shell_sort_pass is not intended to be used outside pg_shell_sort > and ph_insertion_sort, so I think, stealing from their context is ok. > Nonetheless, done.
Looks good. Marking this patch as ready for committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers