-
Notifications
You must be signed in to change notification settings - Fork 280
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
Update SimpleWebServer to the latest version(29 Apr, 2019 - d6985ed6) #691
Conversation
withsmilo
commented
May 12, 2019
•
edited
Loading
edited
- related issue : [Bug] SIGSEGV occurred in the SimpleWebServer very rarely #551
- Diff between original SimpleWebServer(left) and new SimpleWebServer modified for Clipper(right) is here.
Test FAILed. |
Test PASSed. |
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.
Thank you for the PR! I appreciate you added [CLIPPER] comment line so that we can easily update the part whenever we update this dependency.
Everything looks good to me, but I have a few questions.
- Aren't we using C++ formatter anymore? I found some parts of code were formatted in the previous SimpleWebServer V2.1.1.
- This version (d6985) is literally the newest commit of SimpleWebServer. Do you think it is stable enough? I am afraid we have to keep updating the version because the newest master is unstable. Is it the version your team is currently using?
}; | ||
|
||
public: | ||
// [CLIPPER] |
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.
I found in the previous code, these were formatted (I believe we ran the formatter before?). We don't use the C++ formatter anymore? @simon-mo @withsmilo
|
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
SOFTWARE. | ||
*/ | ||
|
||
#ifndef SERVER_HTTP_HPP |
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.
Why don't we add a README.md to record the version/revision hash information? Seems like some C++ dependencies keep track of its version in its directory name, but httpserver does not. I believe it is because we have some additional custom code inside. We can also probably mention whenever we update the version of httpserver
, we should remain the parts with // [CLIPPER]
.
Thanks! I'll directly merge this one since test passed already. |
Test PASSed. |