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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers