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

Declare entry points as __cdecl; Add callback param; Allow multiple calls to SHA1DCFinal() #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

axelriet
Copy link

@axelriet axelriet commented Apr 6, 2017

  • Declare API entry points as __cdecl (through a compiler-specific macro).
  • Add a new void* parameter to the callback function.
  • Allow multiple calls to SHA1DCFinal() - enable getting partial hashes.
  • FIX: Callback was never called.

Axel Rietschin added 2 commits April 6, 2017 16:25
@shumow
Copy link
Collaborator

shumow commented Apr 8, 2017

Hi Axel, before we can merge this Pull Request, we have to make sure the continuous integration checks pass. So, please investigate that and make necessary fixes.

However, to just get a jump on code reviewing. Here are my thoughts:

  1. I see the added param to the callback. I think this is a good change, and something i needed to hack to stand up my prototype service as well. Specifically, if you are running in any sort of multithreaded environment, this is a necessary thing you would need have in order to differentiate which threads collisions are occurring in. Making this parameter allows for a broad enough flexibility for users / writers of the callback to define their own behavior i.e. defining their own struct to record a collision. Without this parameter the caller would have to get the identity of the thread to record it this way. We could alternately just define this struct ourselves, and have it be all of the parameters to the callback. I lean towards the more general approach that you have here.
  2. The define SHA1DC_USES_CALLBACK_PARAM -- a good choice to use given that other people may be using the callback. However, as you fixed, the callback wasn't even being called, so as that is already broken, I would rather not add the flexibility and additional complexity of allowing the callback param to have or not have a parameter. So I would ask you to take this define out and just force the callback to use the new param. This is a breaking change and we should use it.
  3. Thanks for fixing the callback not being called.
  4. If you need the cdecl, we do need to add this flexibility. I would prefer to add it as follows:
#ifndef SHA1DC_API #define SHA1DC_API #endif And add the define either in the makefile or the vcxproj files (see the other outstanding pull request that adds vcxproj files.

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