-
Notifications
You must be signed in to change notification settings - Fork 15
Refresh Windows C++ client #38
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
base: master
Are you sure you want to change the base?
Conversation
|
What kind of Linux and Mac testing did you do on this branch? Do we have a Jenkins job to test this? I don't think we do, but Ruth would know more. I'm still looking at this. |
|
At present, the intend of this branch and the changes is not to run on Linux or Mac. As first step it's targetted to run on Windows platform. I will not merging it master at present but the changes may live on this branch or on origin/windows which already exists but is out-dated - last updated almost more than 2 years back. When it goes to master (later?), it will operational on Linux. |
|
Ok. The pull request says you want to merge to master, which seems to be a mistake unless you have tested on Linux. Most of the 321 file changes are in cppunit and other library functions. Is that true? I didn't review these. I only looked at the changes which seemed to be in VoltDB code. Those seemed to be right to me. |
|
Thanks Bill for reviewing the changes. The pull request has been set-up against master so that it's easier to read the changes happening to master logic. The current branch as is can't go in master at present as this has not been tested to run on Linux. |
7bc6a37 to
747d0eb
Compare
747d0eb to
4fcb065
Compare
examples/Voter.cpp
Outdated
| int64_t maxAllowedOutstanding; | ||
|
|
||
| #ifdef _MSC_VER | ||
| // this version may not be correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not correct version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment needs to be cleaned up. Earlier when had looked at it, it seemed incorrect as it does not take into account epoch time similar to gettimeofday(). But later after looking how this routine is used, taking into account epoch is not needed. Will clean up the comment. Thanks
|
review done. |
|
Thanks for the reviewing the changes. I have pushed changes on the review feedback along with clean-up of sources for libevent as it does seem to be needed for Windows C++ client - purpose based on the how project is structured, it looks like libraries are only needed. I have archieved the sources as it contains Windows project too in it so that in future, if needed, the new libraries can be generated. |
fixed a typo
Logic is based on latest C++ client from master and necessary files pulled in from windows branch. Following are updates in this changelist :
Verified voter 32 and 64 bit example runs on Win 7 and Win 10.