Skip to content

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

Merged
SpamapS merged 1 commit intogearman:masterfrom
esabol:issue-278-fix-stl-vector-usage
Apr 6, 2020
Merged

Issue 278: Fix crashing in libtest/http.cc when '-Wp,-D_GLIBCXX_ASSERTIONS' is given#281
SpamapS merged 1 commit intogearman:masterfrom
esabol:issue-278-fix-stl-vector-usage

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).

This was referenced Feb 21, 2020
Closed
Closed
@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

p-alik
p-alik requested changes Feb 21, 2020
View reviewed changes
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

gearmand/libtest/vchar.hpp

Line 50 in a6bd168

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

Reviewers

@SpamapS SpamapS SpamapS approved these changes

@p-alik p-alik p-alik approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@esabol @cheese @p-alik @SpamapS