Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support

On 1/23/17, Haribabu Kommi <kommi.haribabu@gmail.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).