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

Implement move operations for HTTPClient #8236

Closed
wants to merge 5 commits into from
Closed

Implement move operations for HTTPClient #8236

wants to merge 5 commits into from

Conversation

paulocsanz
Copy link
Contributor

Fixes #8231

This implements move constructor and assignment for HTTPClient, intended to support std::optional proper usage. It's not clear to me if this is the right place to test for compilation errors.

@mcspr
Copy link
Collaborator

mcspr commented Jul 24, 2021

Late sorry for not being clear, since I did not literally mean to implement the move yourself with every member, and it should be apparent it is not really... nice looking and there is a better way to handle this.

Just for consideration: master...mcspr:ptr-wrap-test
(and strictly solving the problem at hand, idk if I'd call it something nice either. real change I'd like would touch much more stuff, this is just to keep most of existing code touching the client and headers)
With the idea to have a protocol for certain members handled through the member class itself, and not to manually micro-manage everything.

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jul 24, 2021

Your changes seem way better! Since you already implemented it, wouldn't you prefer to provide a PR and I close this one?

Just a suggestion, a name like ObserverPtr/NonOwningPtr seem more intuitive than ClientWrapper.

It seems a std::unique_ptr with a no-op deleter would work as well, but seem hackish.

My first instinct was using std::optional<std::reference_wrapper<WiFiClient>>, but it may be overkill to bring all those dependencies for this small feature.

@paulocsanz paulocsanz closed this Jul 24, 2021
@paulocsanz paulocsanz deleted the add/httpclient-move-ops branch July 24, 2021 21:06
mcspr added a commit that referenced this pull request Oct 13, 2021
- =default for default ctor, destructor, move ctor and the assignment move
- use `std::unique_ptr<WiFiClient>` instead of raw pointer to the client
- implement `virtual std::unique_ptr<WiFiClient> WiFiClient::clone()` to safely copy the WiFiClientSecure instance, without accidentally slicing it (i.e. using the pointer with incorrect type, calling base WiFiClient virtual methods)
- replace headers pointer array with `std::unique_ptr<T[]>` to simplify the move operations
- substitute userAgent with the default one when it is empty
(may be a subject to change though, b/c now there is a global static `String`)

Allow HTTPClient to be placed inside of movable classes (e.g. std::optional, requested in the linked issue) or to be returned from functions. Class logic stays as-is, only the underlying member types are changed.

Notice that WiFiClient connection object is now copied, and the internal ClientContext will be preserved even after the original WiFiClient object was destroyed.

replaces #8236
resolves #8231
and, possibly #5734
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.

Unable to set HTTPClient to a std::optional
2 participants