Open axelriet opened 7 years ago
axelriet
commented
7 years ago 
shumow
commented
7 years ago 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:
And add the define either in the makefile or the vcxproj files (see the other outstanding pull request that adds vcxproj files.