HI Ben, >On Wed, Nov 08, 2017 at 04:35:52PM +0000, Bhanuprakash Bodireddy wrote: >> This patchset introduces high resolution sleep support for linux and >windows. >> Also time macros are introduced to replace the numbers with meaningful >> names. > >Thank you very much for the series. > >Did you test that the Windows version of the code compiles (e.g. via >appveyor)?
I cross checked with appveyor and the build was successful. I replied to another thread where we were discussing about the windows implementation. > >I would normally squash patch 3 (the Windows version of xnanosleep) into >patch 2 (the Linux version). I couldn't verify the functionality of windows implementation and hence posted it as a separate patch for now. I will squash it once I receive feedback from Alin. Also, I would normally squash the patches that >just replace constants by xSEC_PER_ySEC macros into the patch that >introduced those macros (if there are other changes then I would separate >those). Ok. > >I am concerned about types. The xSEC_PER_ySEC macros all use type "long" >for their constants, but in some cases the code needs to have type "long >long", for example in many cases when multiplying by one of these macros. >When the patches replace an LL-suffixed literal by one of the xSEC_PER_ySEC >macros, this introduces a risk of overflow that was not present before. > >I am not certain that the xSEC_PER_ySEC macros clarify things, especially given >the type issues. I don't feel strongly about it though. Yeah I understand your concern here and difficult to test the cases for overflow with this changes. I will leave it the way it is now. > >In the xnanosleep() implementation for Windows, I think that the two calls to >CloseHandle can be consolidated into one. Sure. - Bhanuprakash. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
