Code review comment for lp://qastaging/~mir-team/mir/160bit-hash-mac

=== modified file 'include/client/mir/events/event_builders.h'

This breaks ABI, we need to keep compatibility overloads. (Or, if we chose to "get away with it" at least confirm that qtmir and unity8 builds are unaffected.)

~~~~

- mev.mac = mac;
+ memcpy(&mev.mac, mac.data(), sizeof(mev.mac));

(Many times) there's no need, the assignment operator does the right thing. (And, while it likely works, have you checked that the standardese supports memcpy of std::array<>?)

~~~~

+ int count = 0;
+ for (size_t i = 0; i < sizeof(calculated_mac); i++)
+ {
+ if (calculated_mac[i] == cookie.mac[i])
+ {
+ count++;
+ }
+ }
+
+ return count == sizeof(cookie.mac);

"return !memcmp(calculated_mac, cookie.mac, sizeof(calculated_mac));" is more readable (and likely more efficient).

~~~~

I'm not sure of the "right fix" here, but...

There's a lot of repetition of "std::array<uint8_t, 20>". It isn't an accident that they are the same, so give it a name (using Mac = std::array<uint8_t, 20>). Or, as it would be convenient to be compatible with C, it might be better with:

    // MAC is a 160bit sha1 hash
    typedef struct MirMac { uint8_t data[20]; } MirMac;

in a mircookie header.