On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <vitaly.buro...@gmail.com> wrote:
> On 1/23/17, Haribabu Kommi <kommi.harib...@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.
>
Thanks for the 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 */
> ...
>
Changed accordingly.
> 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).
Corrected as suggested.
Updated patch attached. There is no change in the contrib patch.
Regards,
Hari Babu
Fujitsu Australia
mac_eui64_support_8.patch
Description: Binary data
contrib_macaddr8_1.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
