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

WString: mark move ctor as noexcept #7610

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Sep 22, 2020

ref.

Move constructors of all the types used with STL containers, for
example, need to be declared noexcept. Otherwise STL will choose copy
constructors instead. The same is valid for move assignment operations.

With a basic example based on the master

diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp
index 46aad5d5..1c93d8f2 100644
--- a/cores/esp8266/WString.cpp
+++ b/cores/esp8266/WString.cpp
@@ -225,6 +225,7 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) {

 #ifdef __GXX_EXPERIMENTAL_CXX0X__
 void String::move(String &rhs) {
+    Serial.printf("moved! %p -> %p\n", &rhs, this);
     if (buffer()) {
         if (capacity() >= rhs.len()) {
             memmove_P(wbuffer(), rhs.buffer(), rhs.length() + 1);
#include <Arduino.h>

#include <vector>

void setup() {
    Serial.begin(115200);

    std::vector<String> objects;
    for (int num = 0; num < 10; ++num) {
        objects.emplace_back(num); 
    }
}

void loop() { delay(1000); ESP.restart(); }

Since the vector is not using .reserve(...), we need to realloc + move existing vector contents every push when vector needs more capacity. Without the patch, we will never see the moved! string from the above.

Core does use std::vector<String> in a couple of places as well, it is not just possible user code:

git grep 'std::vector<Str'
libraries/ESP8266WebServer/src/Uri.h:        virtual bool canHandle(const String &requestUri, __attribute__((unused)) std::vector<String> &pathArgs) {
libraries/ESP8266WebServer/src/detail/RequestHandler.h:    std::vector<String> pathArgs;
libraries/ESP8266WebServer/src/uri/UriBraces.h:        bool canHandle(const String &requestUri, std::vector<String> &pathArgs) override final {
libraries/ESP8266WebServer/src/uri/UriGlob.h:        bool canHandle(const String &requestUri, __attribute__((unused)) std::vector<String> &pathArgs) override final {
libraries/ESP8266WebServer/src/uri/UriRegex.h:        bool canHandle(const String &requestUri, std::vector<String> &pathArgs) override final {

(But, I am not sure what is the implication for the overall Arduino API stuff. Does it need the patch, too, if this is valid?)

ref.
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-move-noexcept
- https://rules.sonarsource.com/cpp/RSPEC-5018?search=noexecept
- https://clang.llvm.org/extra/clang-tidy/checks/performance-noexcept-move-constructor.html

> Move constructors of all the types used with STL containers, for
example, need to be declared noexcept. Otherwise STL will choose copy
constructors instead. The same is valid for move assignment operations.
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

One of things that is pending is to mark methods as noexcept across the entire code base. This is a good start, and for very good reason.

@devyte
Copy link
Collaborator

devyte commented Sep 23, 2020

Also, if something breaks, now is the time for it, the sooner the better. There is time to detect and figure out such cases.

@earlephilhower earlephilhower merged commit c24109f into esp8266:master Sep 24, 2020
@mcspr mcspr deleted the noe/string branch September 24, 2020 03:37
@mcspr
Copy link
Collaborator Author

mcspr commented Sep 24, 2020

One of things that is pending is to mark methods as noexcept across the entire code base. This is a good start, and for very good reason.

Any hints as to where to look / any specific cases besides T(T&&) and operator=(T&&)
?
Only WiFiClient caught my eye recently. And there is also a logic bug where move ctor is implicitly created, std::is_move_constructible<WiFiClient> is true, causing old object destructor to delete kind-of 'moved' ClientContext pointer.

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.

None yet

3 participants