Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No C bindings for HighwayTreeHash and SipHash #11

Closed
vks opened this issue Jun 22, 2016 · 9 comments
Closed

No C bindings for HighwayTreeHash and SipHash #11

vks opened this issue Jun 22, 2016 · 9 comments

Comments

@vks
Copy link
Contributor

vks commented Jun 22, 2016

It used to be possible to bind against these, however this is no longer the case.

@jan-wassenberg
Copy link
Member

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?

@vks
Copy link
Contributor Author

vks commented Jun 22, 2016

I think that would be the best solution.

@jan-wassenberg
Copy link
Member

Glad to hear it, thanks :)

@vks
Copy link
Contributor Author

vks commented Jun 23, 2016

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).

@jan-wassenberg
Copy link
Member

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 :)

@vks
Copy link
Contributor Author

vks commented Jun 23, 2016

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.

@jan-wassenberg
Copy link
Member

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

jan-wassenberg added a commit that referenced this issue Jun 30, 2016
@jan-wassenberg
Copy link
Member

Done :)

@vks
Copy link
Contributor Author

vks commented Jun 30, 2016

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants