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.rouh...@dalibo.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 >
0003-Implement-SortSupport-for-macaddr-data-type.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
