Re: [PATCH] SortSupport for macaddr type

Hi Julien,

Thanks for the expedient reply, even after I'd dropped the
ball for so long :)

> Indeed, I should have checked more examples :/ There
> isn't any clear pattern for this, so I guess any one
> would be ok.

Yeah, agreed. If it's alright with you, I ended up moving
the naming back to `macaddr_cmp_internal` just because it
results in a smaller final diff.

> Thanks. I'm ok with this, but maybe a native english
> speaker would have a better opinion on this.

Cool! I just re-read my own comment a few days later and I
think that it still mostly makes sense, but definitely open
to other edits if anyone else has one.

> One last thing, I noticed that you added:
>
> +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);
>
> but the existing function is declared as
>
> static int32
> macaddr_internal_cmp(macaddr *a1, macaddr *a2)
>
> I'd be in favor to declare both as int.

Great catch! I have no idea how I missed that. I've done as
you suggested and made them both "int", which seems
consistent with SortSupport implementations elsewhere.

> After this, I think this patch will be ready for committer.

Excellent! I've attached a new (and hopefully final)
version of the patch.

Two final questions about the process if you'd be so kind:

* Should I change the status on the Commitfest link [1] or
do I leave that to you (or someone else like a committer)?

* I've been generating a new OID value with the
`unused_oids` script, but pretty much every time I rebase
I collide with someone else's addition and need to find a
new one. Is it better for me to pick an OID in an exotic
range for my final patch, or that a committer just finds
a new one (if necessary) as they're putting it into
master?

Thanks again!
Brandur

[1] https://commitfest.postgresql.org/10/743/

On Sat, Feb 25, 2017 at 9:56 AM, Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com>
wrote:

> On Sun, Feb 05, 2017 at 01:56:41PM -0800, Brandur Leach wrote:
>
> Hello Brandur, thanks for the updated patch!
>
> >
> > > * you used macaddr_cmp_internal() function name, for uuid
> > > the same function is named uuid_internal_cmp(). Using
> > > the same naming pattern is probably better.
> >
> > I was a little split on this one! It's true that UUID uses
> > `_internal_cmp`, but `_cmp_internal` is also used in a
> > number of places like `enum`, `timetz`, and `network`. I
> > don't have a strong feeling about it either way, so I've
> > changed it to `_internal_cmp` to match UUID as you
> > suggested.
> >
>
> Indeed, I should have checked more examples :/ There isn't any clear
> pattern
> for this, so I guess any one would be ok.
>
> > > * the function comment on macaddr_abbrev_convert()
> > > doesn't mention specific little-endian handling
> >
> > I tried to bake this into the comment text. Here are the
> > relevant lines of the amended version:
> >
> > * Packs the bytes of a 6-byte MAC address into a Datum and treats it
> as
> > an
> > * unsigned integer for purposes of comparison. On a 64-bit machine,
> > there
> > * will be two zeroed bytes of padding. The integer is converted to
> > native
> > * endianness to facilitate easy comparison.
> >
> > > * "There will be two bytes of zero padding on the least
> > > significant end"
> > >
> > > "least significant bits" would be better
> >
> > Also done. Here is the amended version:
> >
> > * On a 64-bit machine, zero out the 8-byte datum and copy the 6
> bytes of
> > * the MAC address in. There will be two bytes of zero padding on the
> end
> > * of the least significant bits.
> >
>
> Thanks. I'm ok with this, but maybe a native english speaker would have a
> better opinion on this.
>
> > > * This patch will trigger quite a lot modifications after
> > > a pgindent run. Could you try to run pgindent on mac.c
> > > before sending an updated patch?
> >
> > Good call! I've run the new version through pgindent.
> >
>
> Thanks also, no more issue here.
>
> > Let me know if you have any further feedback and/or
> > suggestions. Thanks!
>
> One last thing, I noticed that you added:
>
> +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);
>
> but the existing function is declared as
>
> static int32
> macaddr_internal_cmp(macaddr *a1, macaddr *a2)
>
> I'd be in favor to declare both as int.
>
> After this, I think this patch will be ready for committer.
>
> --
> Julien Rouhaud
> http://dalibo.com - http://dalibo.org
>