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

Unable to set HTTPClient to a std::optional #8231

Closed
5 of 6 tasks
paulocsanz opened this issue Jul 23, 2021 · 4 comments · Fixed by #8237
Closed
5 of 6 tasks

Unable to set HTTPClient to a std::optional #8231

paulocsanz opened this issue Jul 23, 2021 · 4 comments · Fixed by #8237

Comments

@paulocsanz
Copy link
Contributor

paulocsanz commented Jul 23, 2021

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: ESP-12
  • Core Version: 3.0.1
  • Development Env: Platformio
  • Operating System: Ubuntu

Problem Description

We are unable to set a HTTPClient to a std::optional that already exists. I have no idea why, the compiler generates hundreds of lines of error all related to the fact that std::optional<HTTPClient>::operator=(std::optional<HTTPClient>&&) is implicitly deleted. I have no idea how to make it work, nor am able to provide a PR to fix this (although I'm willing if pointed in the right direction). But this seems a bug. And is something we actually need (we memset the unused 4kb sys stack to zero and cast it to a struct that contains a bunch of classes, and the easier way to properly initialize them is to wrap them in an optional).

MCVE Sketch

#include <ESP8266HTTPClient.h>
#include <optional>

void setup() {
    std::optional<HTTPClient> http;
    http = std::make_optional(HTTPClient());
}

void loop() {

}

Debug Messages

It's a huge compiler error (it's not from the example, it was extracted from our codebase, we were talking about this issue in the gitter chat): https://pastebin.com/2NZzvrgY

The core of it seems this:

include/loop.hpp:189:57: error: use of deleted function 'std::optional<HTTPClient>& std::optional<HTTPClient>::operator=(std::optional<HTTPClient>&&)'
  189 |       this->data->http = std::make_optional(HTTPClient());

.platformio/packages/toolchain-xtensa@2.100200.0/xtensa-lx106-elf/include/c++/10.2.0/optional:659:11: note: 'std::optional<HTTPClient>& std::optional<HTTPClient>::operator=(std::optional<HTTPClient>&&)' is implicitly deleted because the default definition would be ill-formed:
  659 |     class optional
      |           ^~~~~~~~

.platformio/packages/toolchain-xtensa@2.100200.0/xtensa-lx106-elf/include/c++/10.2.0/optional:659:11: error: use of deleted function 'std::_Enable_copy_move<false, false, false, false, _Tag>& std::_Enable_copy_move<false, false, false, false, _Tag>::operator=(std::_Enable_copy_move<false, false, false, false, _Tag>&&) [with _Tag = std::optional<HTTPClient>]'

.platformio/packages/toolchain-xtensa@2.100200.0/xtensa-lx106-elf/include/c++/10.2.0/bits/enable_special_members.h:306:5: note: declared here
  306 |     operator=(_Enable_copy_move&&) noexcept                         = delete;

.platformio/packages/toolchain-xtensa@2.100200.0/xtensa-lx106-elf/include/c++/10.2.0/optional: In instantiation of 'constexpr std::optional<typename std::decay<_Tp>::type> std::make_optional(_Tp&&) [with _Tp = HTTPClient; typename std::decay<_Tp>::type = std::decay<HTTPClient>::type]':
include/loop.hpp:189:57:   required from here .platformio/packages/toolchain-xtensa@2.100200.0/xtensa-lx106-elf/include/c++/10.2.0/optional:1207:62: error: no matching function for call to 'std::optional<HTTPClient>::optional(<brace-enclosed initializer list>)'
 1207 |     { return optional<decay_t<_Tp>> { std::forward<_Tp>(__t) }; }

And it goes on and on.

@d-a-v
Copy link
Collaborator

d-a-v commented Jul 23, 2021

I tried with g++11 and the message is short and different:

arduino/8231//8231.ino: In function ‘void setup()’:
arduino/8231//8231.ino:8:30: error: no matching function for call to ‘make_optional(HTTPClient)’
    8 |     http = std::make_optional(HTTPClient());
      |            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from arduino/8231//8231.ino:4,
                 from arduino/8231/8231.ino.cpp:1:
/usr/include/c++/11/optional:1261:5: note: candidate: ‘template<class _Tp> constexpr std::enable_if_t<is_constructible_v<typename std::decay<_Tp>::type, _Tp>, std::optional<typename std::decay<_Tp>::type> > std::make_optional(_Tp&&)’
 1261 |     make_optional(_Tp&& __t)
      |     ^~~~~~~~~~~~~
[...]

HTTPClient does not have any move contructor. Did you try to add one ?

@paulocsanz
Copy link
Contributor Author

No I haven't, I can do that tonight. But why should it be implicitly deleted? I'm not that keen on rules on default move constructors.

@mcspr
Copy link
Collaborator

mcspr commented Jul 23, 2021

Quoting earlier gitter thread:

https://gitter.im/esp8266/Arduino?at=60f7e54a7331d202b5c10f43

https://godbolt.org/z/PTPabM1z3, try to =delete things and see the same error

...and the helper class for optional seems to hide the actual error behind this check https://github.com/gcc-mirror/gcc/blob/e0335bb7d1fc7dd05e91bcdd1f65b2bcf8ec1a09/libstdc%2B%2B-v3/include/std/optional#L673, then https://github.com/gcc-mirror/gcc/blob/e0335bb7d1fc7dd05e91bcdd1f65b2bcf8ec1a09/libstdc%2B%2B-v3/include/bits/enable_special_members.h#L107 and downwards it deletes assignment

and also referencing https://en.cppreference.com/w/cpp/language/default_constructor and https://en.cppreference.com/w/cpp/language/move_assignment, you must implement constructors and assignment operators yourself if compiler can't do it for you. For the above, HTTPClient is cannot pass the __and_v<is_move_constructible<_Tp>, is_move_assignable<_Tp>> check. So these need to be manually implemented:

HTTPClient::HTTPClient(HTTPClient&&);
HTTPClient& HTTPClient::operator=(HTTPClient&&);

My earlier suggestion was to also look at WifiClient, since it can't (?) be moved properly. There are issues with ClientContext* member deleting itself though, but that's whole other can of worms edit: read as - cannot be moved with =default and needs explicit _client = nullptr; so it does not end up unref()'ing the pointer)

@paulocsanz
Copy link
Contributor Author

Done!

mcspr added a commit that referenced this issue 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
3 participants