Re: A qsort template

From: Zhihong Yu <zyu(at)yugabyte(dot)com>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Daniel Gustafsson <daniel(at)yesql(dot)se>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: A qsort template
Date: 2021-06-16 22:05:24
Message-ID: CALNJ-vRzf+TfoY_sKsPCb4+3Z4EcdRmZnM69CJkFEV7fdo8+Dg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Jun 16, 2021 at 2:54 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:

> Hi Zhihong,
>
> On Thu, Jun 17, 2021 at 8:13 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> > In 0001-Add-bsearch-and-unique-templates-to-sort_template.h.patch :
> >
> > - const ST_ELEMENT_TYPE *
> ST_SORT_PROTO_ARG);
> > + const ST_ELEMENT_TYPE
> *ST_SORT_PROTO_ARG);
> >
> > It seems there is no real change in the line above. Better keep the
> original formation.
>
> Hmm, well it was only recently damaged by commit def5b065, and that's
> because I'd forgotten to put ST_ELEMENT_TYPE into typedefs.list, and I
> was correcting that in this patch. (That file is used by
> pg_bsd_indent to decide if an identifier is a type or a variable,
> which affects whether '*' is formatted like a unary operator/type
> syntax or a binary operator.)
>
> > * - ST_COMPARE_ARG_TYPE - type of extra argument
> > *
> > + * To say that the comparator returns a type other than int, use:
> > + *
> > + * - ST_COMPARE_TYPE - an integer type
> >
> > Since the ST_COMPARE_TYPE is meant to designate the type of the return
> value, maybe ST_COMPARE_RET_TYPE would be better name.
> > It also goes with ST_COMPARE_ARG_TYPE preceding this.
>
> Good idea, will do.
>
> > - ST_POINTER_TYPE *a = (ST_POINTER_TYPE *) data,
> > - *pa,
> > - *pb,
> > - *pc,
> > - *pd,
> > - *pl,
> > - *pm,
> > - *pn;
> > + ST_POINTER_TYPE *a = (ST_POINTER_TYPE *) data;
> > + ST_POINTER_TYPE *pa;
> >
> > There doesn't seem to be material change for the above hunk.
>
> In master, you can't write #define ST_ELEMENT_TYPE some_type *, which
> seems like it would be quite useful. You can use pointers as element
> types, but only with a typedef name due to C parsing rules. some_type
> **a, *pa, ... declares some_type *pa, but we want some_type **pa. I
> don't want to have to introduce extra typedefs. The change fixes that
> problem by not using C's squirrelly variable declaration list syntax.
>
> > + while (left <= right)
> > + {
> > + size_t mid = (left + right) / 2;
> >
> > The computation for midpoint should be left + (right-left)/2.
>
> Right, my way can overflow. Will fix. Thanks!
>

Hi,
Thanks for giving me background on typedefs.
The relevant changes look fine to me.

Cheers

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2021-06-16 22:13:16 Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
Previous Message John Naylor 2021-06-16 22:02:32 Re: a path towards replacing GEQO with something better