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

Expose the C hash functions #10

Merged
merged 5 commits into from
Dec 25, 2014
Merged

Conversation

wush978
Copy link
Contributor

@wush978 wush978 commented Dec 23, 2014

wush978/FeatureHashing#26

  • register the C functions in init.c
  • A prototype of helper header of PMurHash32

@eddelbuettel
Copy link
Owner

Looks like a very good start!

I am not sure I would declare everything in init.c when I only really export one header. Shall we limit it to just pmurmurhash,h for now (so that you get going) and then figure out the best way of doing things for a new digest 0.7.0, say?

And if I may: as you took inspiration from xts's header, would you mind adding me to the copyright header?

@wush978
Copy link
Contributor Author

wush978 commented Dec 23, 2014

Oh, I originally want to deal with all C hash functions. Limiting to murmurhash sounds good since I am not going to test the others recently. And yes, I would add you to the copyright headers to src/init.c and the inst/include/pmurhash.h. Sorry that I am still new to copyright issues.

I'll submit the changes after Xmas Day.

Happy Xmas Day

@eddelbuettel
Copy link
Owner

I think we are on the same page. I also want to make all the C level functions available (at the source level) to other R packages. But the currently chosen method is probably the most laborious (yet trusted) so I'd say let do this one now for just pmurmurhash. I can also quickly edit this at my end.

- add header guard to exported header
@wush978
Copy link
Contributor Author

wush978 commented Dec 25, 2014

This might be what we need so far. I test the exposed pmurhash.h with FeatureHashing and it passes my unit tests on OS X, Windows and Ubuntu.

eddelbuettel added a commit that referenced this pull request Dec 25, 2014
Expose the C hash functions
@eddelbuettel eddelbuettel merged commit a396967 into eddelbuettel:master Dec 25, 2014
@eddelbuettel
Copy link
Owner

That looks pretty good -- thanks for the update! I will give this one more look-over, try to address one open issue with pmurmurhash (see #9) and then ship this to CRAN so that you can depend on it.

A bit more medium term, we will work out the best way to expose functions (which may just be what you had in init.c, or one of the other previously discussed methods) and maybe create an example package using it.

@eddelbuettel
Copy link
Owner

Made a minor edit in init.c, added something in ChangeLog for your changes and bumped Date: and Version:

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

Successfully merging this pull request may close these issues.

2 participants