Re: macaddr 64 bit (EUI-64) datatype support

On 1/23/17, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> The patch is split into two parts.
> 1. Macaddr8 datatype support
> 2. Contrib module support.

Hello,

I'm sorry for the delay.
The patch is almost done, but I have two requests since the last review.

1.
src/backend/utils/adt/mac8.c:
+ int a,
+ b,
+ c,
+ d = 0,
+ e = 0,
...

There is no reason to set them as 0. For EUI-48 they will be
reassigned in the "if (count != 8)" block, for EUI-64 -- in one of
sscanf.
They could be set to "d = 0xFF, e = 0xFE," and avoid the "if" block
mentioned above, but it makes the code be much less readable.

Oh. I see. In the current version it must be assigned because for
EUI-48 memory can have values <0 or >255 in uninitialized variables.
It is better to avoid initialization by merging two parts of the code:
+ if (count != 6)
+ {
+ /* May be a 8-byte MAC address */
...
+ if (count != 8)
+ {
+ d = 0xFF;
+ e = 0xFE;
+ }

to a single one:
+ if (count == 6)
+ {
+ d = 0xFF;
+ e = 0xFE;
+ }
+ else
+ {
+ /* May be a 8-byte MAC address */
...

2.
src/backend/utils/adt/network.c:
+ res = (mac->a << 24) | (mac->b << 16) | (mac->c << 8) | (mac->d);
+ res = (double)((uint64)res << 32);
+ res += (mac->e << 24) | (mac->f << 16) | (mac->g << 8) | (mac->h);

Khm... I trust that modern compilers can do a lot of optimizations but
for me it looks terrible because of needless conversions.
The reason why earlier versions did have two lines "res *= 256 * 256"
was an integer overflow for four multipliers, but it can be solved by
defining the first multiplier as a double:
+ res = (mac->a << 24) | (mac->b << 16) | (mac->c << 8) | (mac->d);
+ res *= (double)256 * 256 * 256 * 256;
+ res += (mac->e << 24) | (mac->f << 16) | (mac->g << 8) | (mac->h);

In this case the left-hand side argument for the "*=" operator is
computed at the compile time as a single constant.
The second line can be written as "res *= 256. * 256 * 256 * 256;"
(pay attention to a dot in the first multiplier), but it is not
readable at all (and produces the same code).

--
Best regards,
Vitaly Burovoy