No C bindings for HighwayTreeHash and SipHash about highwayhash HOT 9 CLOSED

Hi Vinzent, we were keen on simplifying the headers (already complicated due to conditional compilation and *State classes). How about this approach using a separate header?

from highwayhash.

I think that would be the best solution.

from highwayhash.

Glad to hear it, thanks :)

from highwayhash.

The bindings seem a bit inconsistent:

• ScalarHighwayTreeHashC and SSE41HighwayTreeHashC seem to duplicate ScalarHighwayTreeHash and SSE41HighwayTreeHash. I think only the ones with a C suffix should exist?
• There is ScalarSipTreeHash and SipTreeHash. They should probably get a C suffix as well?
• HighwayTreeHashC was not callable from C before, so it's nice to have it.
• There is no SipHashC.

I'll test the bindings once I get the build working again (#12).

from highwayhash.

The C-suffix functions have two disadvantages: they're harder to inline (requires link-time inlining because they're not in the header) and the key parameter is not type-safe (can cause out of bounds accesses - SipHash expects 2 uint64, the others 4). That's why we'd like to keep the C bindings separate :)

Honestly, I don't see much value in SipTreeHash because it's not drop-in compatible with SipHash, nor is it as fast as HighwayHash. Still, we can add their bindings for completeness (plus SipHashC - thanks for pointing that out). Please feel free to re-open if needed :)

from highwayhash.

I see. I bound against the non-suffix functions and it worked, probably due to an implicit cast.

Looks good to me, except that SipHashC seems to be missing from c_bindings.h.

from highwayhash.

Thanks for looking, sorry about the missing SipHashC. Will add that next week :)

from highwayhash.

Done :)

from highwayhash.

Great, thanks!

from highwayhash.