On Mon, Jun 3, 2019 at 2:39 PM Peter Geoghegan <pg@bowt.ie> wrote:
As you know, it's a bit weird that we're proposing adding sort support with abbreviated keys for a type that is 8 bytes, since you'd expect it to also be pass-by-value on most platforms, which largely defeats the purpose of having abbreviated keys (though sort support could still make sense, for the same reason it makes sense to have it for int8). However, macaddr8 isn't actually pass-by-value, and it seems too late to do anything about that now, so abbreviated keys actually make sense.
so, if what you say is true and it is either not worth it or potentially a breaking change to make macaddr8 pass-by-value and adding abbreviated sort support has the potential to avoid "pointer chasing" and guarantee equivalent relative performance for macaddr8 and macaddr, then that seems worth it.
With regard to macaddr8_abbrev_convert() and memset(), I attached a patch with the memset() removed, since it is superfluous here.
macaddr8_cmp_internal() already existed before this patch and I noticed that it explicitly returns int32 whereas the return type of macaddr_cmp_internal() is just specified as an int. I was wondering why.
I also noticed that the prototype for macaddr8_cmp_internal() was not at the top of the file with the other static function prototypes. I added it there, but I wasn't sure if there was some reason that it was like that to begin with.
