-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add Client methods with both content provider and receiver #2268
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
Conversation
|
@clarkok thanks for the pull request. But I am not going to approve it since it create another two POST specific implementations. Could you please rename |
|
Thanks @yhirose . I've done the changes you mentioned. Would you mind taking a look? |
httplib.h
Outdated
|
|
||
| // Allocate on the heap, so the resolver thread can keep using the data. | ||
| auto state = std::make_shared<GetAddrInfoState>(); | ||
| state->node = node; |
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.
Could you explain why these lines need to be added? These are already added at line 3842 and 3843.
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.
Sorry I was not planning to take this change in the PR, I'm not very familar with how PR works in Github.
However this is a separated issue I found during my Android project. In short, this doesn't work, the node on the right side of the assignment is actually referring to the member of GetAddrInforState which is empty, instead of the function parameter. Thus the getaddrinfo below always return EAI_SERVICE as both node and service are empty, and probably hints are not in expected state.
I've written a short piece of repro code in the below link for your reference.
https://godbolt.org/z/9sW9EjjYK
I'll revert this back and create a new PR for the issue.
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.
Thanks for the explanation!
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.
NP, BTW after this fix the library is working on Android flawlessly
|
@clarkok Thanks for the excellent contribution! |
Add most
Postoverloads forClient, with both content provider and content receiver.