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

Issue 278: Fix crashing in libtest/http.cc when '-Wp,-D_GLIBCXX_ASSERTIONS' is given #281

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

esabol
Copy link
Member

@esabol esabol commented Feb 21, 2020

This PR fixes the problems in libtest/http.cc when you compile with CXXFLAGS=-'-Wp,-D_GLIBCXX_ASSERTIONS' on Fedora 31. Refer to issue #278 (and issue #275).

@esabol
Copy link
Member Author

esabol commented Feb 21, 2020

Please restart the following Travis CI jobs:
https://travis-ci.org/gearman/gearmand/jobs/653265367
https://travis-ci.org/gearman/gearmand/jobs/653265370
I believe the test failures with these are spurious. Thanks!

@cheese
Copy link

cheese commented Feb 21, 2020

LGTM

Copy link
Collaborator

@p-alik p-alik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to replace &_body[0] by _body.data() at least in both if (HAVE_LIBCURL && _body.size()) blocks.

@esabol
Copy link
Member Author

esabol commented Feb 21, 2020

It would be good to replace &_body[0] by _body.data() at least in both if (HAVE_LIBCURL && _body.size()) blocks.

Why would that be better? I don't see it.

@p-alik
Copy link
Collaborator

p-alik commented Feb 21, 2020

c++ type should be treated with well-defined type methods

typedef std::vector<char> vchar_t;

In my opinion mixing pure c and c++ style is inappropriate.

@esabol
Copy link
Member Author

esabol commented Feb 21, 2020

In my opinion mixing pure c and c++ style is inappropriate.

Well, that seems to be a separate issue then that should go in a separate PR.

@esabol
Copy link
Member Author

esabol commented Apr 5, 2020

Would it be OK if I merged this?

@p-alik
Copy link
Collaborator

p-alik commented Apr 6, 2020

I've no objections

@SpamapS
Copy link
Member

SpamapS commented Apr 6, 2020

GMTA @p-alik :)

@SpamapS SpamapS merged commit 7976bc2 into gearman:master Apr 6, 2020
@esabol esabol deleted the issue-278-fix-stl-vector-usage branch April 7, 2020 19:33
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.

4 participants