From 9c47a1cb5245af6dc2fe793204caf3734f6f3fb1 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sat, 19 Dec 2020 10:58:47 +0300 Subject: [PATCH 01/16] wip --- cores/esp8266/WString.cpp | 121 ++++++++++++++++---------------------- cores/esp8266/WString.h | 117 +++++++++++++++++------------------- 2 files changed, 106 insertions(+), 132 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 07ab99b2cf..354e19587e 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -21,7 +21,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ -#include +#include "Arduino.h" #include "WString.h" #include "stdlib_noniso.h" @@ -50,11 +50,6 @@ String::String(String &&rval) noexcept { move(rval); } -String::String(StringSumHelper &&rval) noexcept { - init(); - move(rval); -} - String::String(unsigned char value, unsigned char base) { init(); char buf[1 + 8 * sizeof(unsigned char)]; @@ -340,84 +335,70 @@ unsigned char String::concat(const __FlashStringHelper *str) { } /*********************************************/ -/* Concatenate */ +/* Insert */ /*********************************************/ -StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(rhs.buffer(), rhs.len())) - a.invalidate(); - return a; -} - -StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr) { - StringSumHelper &a = const_cast(lhs); - if (!cstr || !a.concat(cstr, strlen(cstr))) - a.invalidate(); - return a; -} +String& String::insert(size_t position, const String& other) { + if (position > length()) { + return *this; + } -StringSumHelper &operator +(const StringSumHelper &lhs, char c) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(c)) - a.invalidate(); - return a; -} + auto total = length() + other.length(); + if (!changeBuffer(total)) + return *this; -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; -} + auto left = length() - position; + setLen(total); -StringSumHelper &operator +(const StringSumHelper &lhs, int num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; -} + auto* start = wbuffer() + position; + memmove(start + other.length(), start, left); + memmove(start, other.c_str(), other.length()); -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; + return *this; } -StringSumHelper &operator +(const StringSumHelper &lhs, long num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +// TODO: it is possible to reuse `other.wbuffer()` when it's capacity is larger than `this->wbuffer()` +String& String::insert(size_t position, String&& other) { + return insert(position, other); } -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; -} +String operator +(const String& lhs, String&& rhs) { + String res; + auto total = lhs.length() + rhs.length(); + if (rhs.capacity() >= total) { + rhs.insert(0, lhs); + res = std::move(rhs); + rhs.setLen(total); + } else { + res.reserve(total + 1); + res += lhs; + res += rhs; + rhs.invalidate(); + } -StringSumHelper &operator +(const StringSumHelper &lhs, float num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; + return res; } -StringSumHelper &operator +(const StringSumHelper &lhs, double num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; -} +String operator +(String&& lhs, String&& rhs) { + String res; + auto total = lhs.length() + rhs.length(); + if (lhs.capacity() >= total) { + res = std::move(lhs); + res += rhs; + rhs.invalidate(); + } else if (rhs.capacity() >= total) { + rhs.insert(0, lhs); + res = std::move(rhs); + res.setLen(total); + } else { + res.reserve(total); + res += lhs; + res += rhs; + lhs.invalidate(); + rhs.invalidate(); + } -StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(rhs)) - a.invalidate(); - return a; + return res; } /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index dcb0982387..6a2066462c 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -23,14 +23,15 @@ #define String_class_h #ifdef __cplusplus -#include -#include -#include #include -// An inherited class for holding the result of a concatenation. These -// result objects are assumed to be writable by subsequent concatenations. -class StringSumHelper; +#include +#include +#include +#include + +#include +#include // an abstract class used as a means to proide a unique pointer type // but really has no body @@ -60,7 +61,6 @@ class String { String(const String &str); String(const __FlashStringHelper *str); String(String &&rval) noexcept; - String(StringSumHelper &&rval) noexcept; explicit String(char c) { sso.buff[0] = c; sso.buff[1] = 0; @@ -100,9 +100,6 @@ class String { String &operator =(const char *cstr); String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; - String &operator =(StringSumHelper &&rval) noexcept { - return operator =((String &&)rval); - } // concatenate (works w/ built-in types) @@ -110,6 +107,7 @@ class String { // is left unchanged). if the argument is null or invalid, the // concatenation is considered unsuccessful. unsigned char concat(const String &str); + unsigned char concat(String &&str); unsigned char concat(const char *cstr); unsigned char concat(char c); unsigned char concat(unsigned char c); @@ -132,6 +130,10 @@ class String { concat(cstr); return *this; } + String &operator +=(const __FlashStringHelper *str) { + concat(str); + return *this; + } String &operator +=(char c) { concat(c); return *this; @@ -164,22 +166,6 @@ class String { concat(num); return *this; } - String &operator +=(const __FlashStringHelper *str) { - concat(str); - return *this; - } - - friend StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs); - friend StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr); - friend StringSumHelper &operator +(const StringSumHelper &lhs, char c); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, int num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, long num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, float num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, double num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs); // comparison (only works w/ Strings and "strings") operator StringIfHelperType() const { @@ -322,6 +308,11 @@ class String { const char *buffer() const { return wbuffer(); } char *wbuffer() const { return isSSO() ? const_cast(sso.buff) : ptr.buff; } // Writable version of buffer + // concatenation is done via non-member functions + // make sure we still have access to internal methods, since we optimize based on capacity of both sides and want to manipulate internal buffers directly + friend String operator +(const String& lhs, String&& rhs); + friend String operator +(String&& lhs, String&& rhs); + protected: void init(void) __attribute__((always_inline)) { sso.buff[0] = 0; @@ -347,45 +338,47 @@ class String { String ©(const char *cstr, unsigned int length); String ©(const __FlashStringHelper *pstr, unsigned int length); void move(String &rhs) noexcept; -}; -class StringSumHelper: public String { - public: - StringSumHelper(const String &s) : - String(s) { - } - StringSumHelper(const char *p) : - String(p) { - } - StringSumHelper(char c) : - String(c) { - } - StringSumHelper(unsigned char num) : - String(num) { - } - StringSumHelper(int num) : - String(num) { - } - StringSumHelper(unsigned int num) : - String(num) { - } - StringSumHelper(long num) : - String(num) { - } - StringSumHelper(unsigned long num) : - String(num) { - } - StringSumHelper(float num) : - String(num) { - } - StringSumHelper(double num) : - String(num) { - } - StringSumHelper(const __FlashStringHelper *s) : - String(s) { - } + // insert at a specific position inside of the string + String& insert(size_t position, const String &); + String& insert(size_t position, String &&); }; +// concatenation (note that it's done using non-method operators to handle both possible type refs) + +inline String operator +(const String& lhs, const String& rhs) { + String res; + res.reserve(lhs.length() + rhs.length() + 1); + res += lhs; + res += rhs; + return res; +} + +inline String operator +(String&& lhs, const String& rhs) { + lhs.concat(rhs); + return std::move(lhs); +} + +String operator +(const String& lhs, String&& rhs); +String operator +(String&& lhs, String&& rhs); + +// concat / += already handles the basic types, so just do that + +template +inline std::enable_if_t && !std::is_same_v, String>, String> +operator +(const String& lhs, T value) { + String res(lhs); + res += value; + return res; +} + +template +inline std::enable_if_t && !std::is_same_v, String>, String> +operator +(String&& lhs, T value) { + lhs += value; + return std::move(lhs); +} + extern const String emptyString; #endif // __cplusplus From d1d584e0dd8edf4b7700da03540e720ada9d51b2 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sat, 19 Dec 2020 11:03:32 +0300 Subject: [PATCH 02/16] huh, turns out String = 'c' did some weird stuff --- cores/esp8266/WString.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 6a2066462c..dc620fe5fe 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -100,6 +100,11 @@ class String { String &operator =(const char *cstr); String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; + String &operator =(char c) { + char buffer[2] { c, '\0' }; + *this = buffer; + return *this; + } // concatenate (works w/ built-in types) From c9a55f78af0e5d044e90fe5808ac6cafacd52235 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 00:47:15 +0300 Subject: [PATCH 03/16] style --- cores/esp8266/WString.cpp | 23 ++++++++--------------- cores/esp8266/WString.h | 15 +++++++-------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 354e19587e..01d77f58cd 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -338,7 +338,7 @@ unsigned char String::concat(const __FlashStringHelper *str) { /* Insert */ /*********************************************/ -String& String::insert(size_t position, const String& other) { +String &String::insert(size_t position, const String& other) { if (position > length()) { return *this; } @@ -357,20 +357,14 @@ String& String::insert(size_t position, const String& other) { return *this; } -// TODO: it is possible to reuse `other.wbuffer()` when it's capacity is larger than `this->wbuffer()` -String& String::insert(size_t position, String&& other) { - return insert(position, other); -} - -String operator +(const String& lhs, String&& rhs) { +String operator +(const String &lhs, String &&rhs) { String res; auto total = lhs.length() + rhs.length(); - if (rhs.capacity() >= total) { + if (rhs.capacity() > total) { rhs.insert(0, lhs); res = std::move(rhs); - rhs.setLen(total); } else { - res.reserve(total + 1); + res.reserve(total); res += lhs; res += rhs; rhs.invalidate(); @@ -382,14 +376,13 @@ String operator +(const String& lhs, String&& rhs) { String operator +(String&& lhs, String&& rhs) { String res; auto total = lhs.length() + rhs.length(); - if (lhs.capacity() >= total) { - res = std::move(lhs); - res += rhs; + if (lhs.capacity() > total) { + lhs += rhs; rhs.invalidate(); - } else if (rhs.capacity() >= total) { + res = std::move(lhs); + } else if (rhs.capacity() > total) { rhs.insert(0, lhs); res = std::move(rhs); - res.setLen(total); } else { res.reserve(total); res += lhs; diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index dc620fe5fe..675d83842c 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -112,7 +112,6 @@ class String { // is left unchanged). if the argument is null or invalid, the // concatenation is considered unsuccessful. unsigned char concat(const String &str); - unsigned char concat(String &&str); unsigned char concat(const char *cstr); unsigned char concat(char c); unsigned char concat(unsigned char c); @@ -351,27 +350,27 @@ class String { // concatenation (note that it's done using non-method operators to handle both possible type refs) -inline String operator +(const String& lhs, const String& rhs) { +inline String operator +(const String &lhs, const String &rhs) { String res; - res.reserve(lhs.length() + rhs.length() + 1); + res.reserve(lhs.length() + rhs.length()); res += lhs; res += rhs; return res; } -inline String operator +(String&& lhs, const String& rhs) { +inline String operator +(String &&lhs, const String &rhs) { lhs.concat(rhs); return std::move(lhs); } -String operator +(const String& lhs, String&& rhs); -String operator +(String&& lhs, String&& rhs); +String operator +(const String &lhs, String&& rhs); +String operator +(String &&lhs, String &&rhs); // concat / += already handles the basic types, so just do that template inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(const String& lhs, T value) { +operator +(const String &lhs, T value) { String res(lhs); res += value; return res; @@ -379,7 +378,7 @@ operator +(const String& lhs, T value) { template inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(String&& lhs, T value) { +operator +(String &&lhs, T value) { lhs += value; return std::move(lhs); } From 14e1e7cb6c5861bb4760b4905634a125cf241cee Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 01:41:05 +0300 Subject: [PATCH 04/16] allow "blah" + String, 'c' + String and F("...") + String also, simplify const char* vs. __FlashStringHelper, and just always use _P functions --- cores/esp8266/WString.cpp | 26 +++++++++++++--- cores/esp8266/WString.h | 64 ++++++++++++++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 01d77f58cd..c4b2df37d2 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -338,12 +338,12 @@ unsigned char String::concat(const __FlashStringHelper *str) { /* Insert */ /*********************************************/ -String &String::insert(size_t position, const String& other) { +String &String::insert(size_t position, const char *other, size_t other_length) { if (position > length()) { return *this; } - auto total = length() + other.length(); + auto total = length() + other_length; if (!changeBuffer(total)) return *this; @@ -351,12 +351,30 @@ String &String::insert(size_t position, const String& other) { setLen(total); auto* start = wbuffer() + position; - memmove(start + other.length(), start, left); - memmove(start, other.c_str(), other.length()); + memmove(start + other_length, start, left); + memmove_P(start, other, other_length); return *this; } +String &String::insert(size_t position, const __FlashStringHelper* other) { + auto* p = reinterpret_cast(other); + return insert(position, p, strlen_P(p)); +} + +String &String::insert(size_t position, char other) { + char tmp[2] { other, '\0' }; + return insert(position, tmp, 1); +} + +String &String::insert(size_t position, const char *other) { + return insert(position, other, strlen(other)); +} + +String &String::insert(size_t position, const String& other) { + return insert(position, other.c_str(), other.length()); +} + String operator +(const String &lhs, String &&rhs) { String res; auto total = lhs.length() + rhs.length(); diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 675d83842c..de1ebb03d9 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -314,8 +314,11 @@ class String { // concatenation is done via non-member functions // make sure we still have access to internal methods, since we optimize based on capacity of both sides and want to manipulate internal buffers directly - friend String operator +(const String& lhs, String&& rhs); - friend String operator +(String&& lhs, String&& rhs); + friend String operator +(const String &lhs, String &&rhs); + friend String operator +(String &&lhs, String &&rhs); + friend String operator +(char lhs, String &&rhs); + friend String operator +(const char *lhs, String &&rhs); + friend String operator +(const __FlashStringHelper *lhs, String &&rhs); protected: void init(void) __attribute__((always_inline)) { @@ -338,14 +341,18 @@ class String { void invalidate(void); unsigned char changeBuffer(unsigned int maxStrLen); - // copy and move + // copy or insert at a specific position String ©(const char *cstr, unsigned int length); String ©(const __FlashStringHelper *pstr, unsigned int length); - void move(String &rhs) noexcept; - // insert at a specific position inside of the string - String& insert(size_t position, const String &); - String& insert(size_t position, String &&); + String &insert(size_t position, char); + String &insert(size_t position, const char *); + String &insert(size_t position, const __FlashStringHelper *); + String &insert(size_t position, const char *, size_t length); + String &insert(size_t position, const String &); + + // rvalue helper + void move(String &rhs) noexcept; }; // concatenation (note that it's done using non-method operators to handle both possible type refs) @@ -367,6 +374,8 @@ String operator +(const String &lhs, String&& rhs); String operator +(String &&lhs, String &&rhs); // concat / += already handles the basic types, so just do that +// TODO: could also do `operator+=` as a templated method +// TODO: explicitly list concat overloads in the enable_if? template inline std::enable_if_t && !std::is_same_v, String>, String> @@ -383,6 +392,47 @@ operator +(String &&lhs, T value) { return std::move(lhs); } +// `String(char)` is explicit, but we used to have StringSumHelper silently allowing the following: +// `String x; x = 'a' + String('b') + 'c';` +// For comparison, `std::string(char)` does not exist. However, we are allowed to concatenate `char` like the example above + +inline String operator +(char lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + 1); + res += lhs; + res += rhs; + return res; +} + +inline String operator +(char lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} + +// both `char*` and `__FlashStringHelper*` are implicitly converted into `String()`, calling the `operator+(const String& ...);` +// however, here we: +// - do an automatic `reserve(total length)` for the resulting string +// - possibly do rhs.insert(0, ...), when &&rhs capacity could fit both + +inline String operator +(const char *lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + strlen_P(lhs)); // TODO: just strlen, remove flashstringhelper optimization below? + res += lhs; + res += rhs; + return res; +} + +inline String operator +(const char *lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} + +inline String operator +(const __FlashStringHelper *lhs, const String &rhs) { + return reinterpret_cast(lhs) + rhs; +} + +inline String operator +(const __FlashStringHelper *lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} + extern const String emptyString; #endif // __cplusplus From 559a5db85c7a5eaf6f21491524ec5b5a9ec802a9 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 01:45:18 +0300 Subject: [PATCH 05/16] shuffle things into .cpp --- cores/esp8266/WString.cpp | 16 ++++++++++++++++ cores/esp8266/WString.h | 16 ++-------------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index c4b2df37d2..a5c6bdd3a5 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -412,6 +412,22 @@ String operator +(String&& lhs, String&& rhs) { return res; } +String operator +(char lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + 1); + res += lhs; + res += rhs; + return res; +} + +String operator +(const char *lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + strlen_P(lhs)); + res += lhs; + res += rhs; + return res; +} + /*********************************************/ /* Comparison */ /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index de1ebb03d9..d748debbb1 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -396,13 +396,7 @@ operator +(String &&lhs, T value) { // `String x; x = 'a' + String('b') + 'c';` // For comparison, `std::string(char)` does not exist. However, we are allowed to concatenate `char` like the example above -inline String operator +(char lhs, const String &rhs) { - String res; - res.reserve(rhs.length() + 1); - res += lhs; - res += rhs; - return res; -} +String operator +(char lhs, const String &rhs); inline String operator +(char lhs, String &&rhs) { return std::move(rhs.insert(0, lhs)); @@ -413,13 +407,7 @@ inline String operator +(char lhs, String &&rhs) { // - do an automatic `reserve(total length)` for the resulting string // - possibly do rhs.insert(0, ...), when &&rhs capacity could fit both -inline String operator +(const char *lhs, const String &rhs) { - String res; - res.reserve(rhs.length() + strlen_P(lhs)); // TODO: just strlen, remove flashstringhelper optimization below? - res += lhs; - res += rhs; - return res; -} +String operator +(const char *lhs, const String &rhs); inline String operator +(const char *lhs, String &&rhs) { return std::move(rhs.insert(0, lhs)); From 6dbcfa9cc35957fb550410926d4bb1afa8f2bf25 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 02:01:08 +0300 Subject: [PATCH 06/16] trying to fix arduinojson based on the implementation, we only need to specify that this symbol is a class --- cores/esp8266/WString.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index d748debbb1..3dbf54b141 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -39,6 +39,10 @@ class __FlashStringHelper; #define FPSTR(pstr_pointer) (reinterpret_cast(pstr_pointer)) #define F(string_literal) (FPSTR(PSTR(string_literal))) +// support libraries that expect this name to be available +// replace with `using StringSumHelper = String;` in case something wants this constructible +class StringSumHelper; + // The string class class String { // use a function pointer to allow for "if (s)" without the From 468392370a67ccb5c4e994f145d482015e1a5501 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 06:25:30 +0300 Subject: [PATCH 07/16] fix accidental realloc, add test for operator+ basic chaining should work just like with master comparing std::move() buffers won't work though, because we never allow anything but `const StringSumHelper&` references --- cores/esp8266/WString.cpp | 136 +++++++++++++------------- cores/esp8266/WString.h | 163 ++++++++++++-------------------- tests/host/core/test_string.cpp | 38 ++++++++ 3 files changed, 164 insertions(+), 173 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index a5c6bdd3a5..07ab99b2cf 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -21,7 +21,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ -#include "Arduino.h" +#include #include "WString.h" #include "stdlib_noniso.h" @@ -50,6 +50,11 @@ String::String(String &&rval) noexcept { move(rval); } +String::String(StringSumHelper &&rval) noexcept { + init(); + move(rval); +} + String::String(unsigned char value, unsigned char base) { init(); char buf[1 + 8 * sizeof(unsigned char)]; @@ -335,97 +340,84 @@ unsigned char String::concat(const __FlashStringHelper *str) { } /*********************************************/ -/* Insert */ +/* Concatenate */ /*********************************************/ -String &String::insert(size_t position, const char *other, size_t other_length) { - if (position > length()) { - return *this; - } - - auto total = length() + other_length; - if (!changeBuffer(total)) - return *this; - - auto left = length() - position; - setLen(total); - - auto* start = wbuffer() + position; - memmove(start + other_length, start, left); - memmove_P(start, other, other_length); - - return *this; +StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(rhs.buffer(), rhs.len())) + a.invalidate(); + return a; } -String &String::insert(size_t position, const __FlashStringHelper* other) { - auto* p = reinterpret_cast(other); - return insert(position, p, strlen_P(p)); +StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr) { + StringSumHelper &a = const_cast(lhs); + if (!cstr || !a.concat(cstr, strlen(cstr))) + a.invalidate(); + return a; } -String &String::insert(size_t position, char other) { - char tmp[2] { other, '\0' }; - return insert(position, tmp, 1); +StringSumHelper &operator +(const StringSumHelper &lhs, char c) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(c)) + a.invalidate(); + return a; } -String &String::insert(size_t position, const char *other) { - return insert(position, other, strlen(other)); +StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; } -String &String::insert(size_t position, const String& other) { - return insert(position, other.c_str(), other.length()); +StringSumHelper &operator +(const StringSumHelper &lhs, int num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; } -String operator +(const String &lhs, String &&rhs) { - String res; - auto total = lhs.length() + rhs.length(); - if (rhs.capacity() > total) { - rhs.insert(0, lhs); - res = std::move(rhs); - } else { - res.reserve(total); - res += lhs; - res += rhs; - rhs.invalidate(); - } +StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; +} - return res; +StringSumHelper &operator +(const StringSumHelper &lhs, long num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; } -String operator +(String&& lhs, String&& rhs) { - String res; - auto total = lhs.length() + rhs.length(); - if (lhs.capacity() > total) { - lhs += rhs; - rhs.invalidate(); - res = std::move(lhs); - } else if (rhs.capacity() > total) { - rhs.insert(0, lhs); - res = std::move(rhs); - } else { - res.reserve(total); - res += lhs; - res += rhs; - lhs.invalidate(); - rhs.invalidate(); - } +StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; +} - return res; +StringSumHelper &operator +(const StringSumHelper &lhs, float num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; } -String operator +(char lhs, const String &rhs) { - String res; - res.reserve(rhs.length() + 1); - res += lhs; - res += rhs; - return res; +StringSumHelper &operator +(const StringSumHelper &lhs, double num) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(num)) + a.invalidate(); + return a; } -String operator +(const char *lhs, const String &rhs) { - String res; - res.reserve(rhs.length() + strlen_P(lhs)); - res += lhs; - res += rhs; - return res; +StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs) { + StringSumHelper &a = const_cast(lhs); + if (!a.concat(rhs)) + a.invalidate(); + return a; } /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 3dbf54b141..dcb0982387 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -23,15 +23,14 @@ #define String_class_h #ifdef __cplusplus +#include +#include +#include #include -#include -#include -#include -#include - -#include -#include +// An inherited class for holding the result of a concatenation. These +// result objects are assumed to be writable by subsequent concatenations. +class StringSumHelper; // an abstract class used as a means to proide a unique pointer type // but really has no body @@ -39,10 +38,6 @@ class __FlashStringHelper; #define FPSTR(pstr_pointer) (reinterpret_cast(pstr_pointer)) #define F(string_literal) (FPSTR(PSTR(string_literal))) -// support libraries that expect this name to be available -// replace with `using StringSumHelper = String;` in case something wants this constructible -class StringSumHelper; - // The string class class String { // use a function pointer to allow for "if (s)" without the @@ -65,6 +60,7 @@ class String { String(const String &str); String(const __FlashStringHelper *str); String(String &&rval) noexcept; + String(StringSumHelper &&rval) noexcept; explicit String(char c) { sso.buff[0] = c; sso.buff[1] = 0; @@ -104,10 +100,8 @@ class String { String &operator =(const char *cstr); String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; - String &operator =(char c) { - char buffer[2] { c, '\0' }; - *this = buffer; - return *this; + String &operator =(StringSumHelper &&rval) noexcept { + return operator =((String &&)rval); } // concatenate (works w/ built-in types) @@ -138,10 +132,6 @@ class String { concat(cstr); return *this; } - String &operator +=(const __FlashStringHelper *str) { - concat(str); - return *this; - } String &operator +=(char c) { concat(c); return *this; @@ -174,6 +164,22 @@ class String { concat(num); return *this; } + String &operator +=(const __FlashStringHelper *str) { + concat(str); + return *this; + } + + friend StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs); + friend StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr); + friend StringSumHelper &operator +(const StringSumHelper &lhs, char c); + friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, int num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, long num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, float num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, double num); + friend StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs); // comparison (only works w/ Strings and "strings") operator StringIfHelperType() const { @@ -316,14 +322,6 @@ class String { const char *buffer() const { return wbuffer(); } char *wbuffer() const { return isSSO() ? const_cast(sso.buff) : ptr.buff; } // Writable version of buffer - // concatenation is done via non-member functions - // make sure we still have access to internal methods, since we optimize based on capacity of both sides and want to manipulate internal buffers directly - friend String operator +(const String &lhs, String &&rhs); - friend String operator +(String &&lhs, String &&rhs); - friend String operator +(char lhs, String &&rhs); - friend String operator +(const char *lhs, String &&rhs); - friend String operator +(const __FlashStringHelper *lhs, String &&rhs); - protected: void init(void) __attribute__((always_inline)) { sso.buff[0] = 0; @@ -345,85 +343,48 @@ class String { void invalidate(void); unsigned char changeBuffer(unsigned int maxStrLen); - // copy or insert at a specific position + // copy and move String ©(const char *cstr, unsigned int length); String ©(const __FlashStringHelper *pstr, unsigned int length); - - String &insert(size_t position, char); - String &insert(size_t position, const char *); - String &insert(size_t position, const __FlashStringHelper *); - String &insert(size_t position, const char *, size_t length); - String &insert(size_t position, const String &); - - // rvalue helper void move(String &rhs) noexcept; }; -// concatenation (note that it's done using non-method operators to handle both possible type refs) - -inline String operator +(const String &lhs, const String &rhs) { - String res; - res.reserve(lhs.length() + rhs.length()); - res += lhs; - res += rhs; - return res; -} - -inline String operator +(String &&lhs, const String &rhs) { - lhs.concat(rhs); - return std::move(lhs); -} - -String operator +(const String &lhs, String&& rhs); -String operator +(String &&lhs, String &&rhs); - -// concat / += already handles the basic types, so just do that -// TODO: could also do `operator+=` as a templated method -// TODO: explicitly list concat overloads in the enable_if? - -template -inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(const String &lhs, T value) { - String res(lhs); - res += value; - return res; -} - -template -inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(String &&lhs, T value) { - lhs += value; - return std::move(lhs); -} - -// `String(char)` is explicit, but we used to have StringSumHelper silently allowing the following: -// `String x; x = 'a' + String('b') + 'c';` -// For comparison, `std::string(char)` does not exist. However, we are allowed to concatenate `char` like the example above - -String operator +(char lhs, const String &rhs); - -inline String operator +(char lhs, String &&rhs) { - return std::move(rhs.insert(0, lhs)); -} - -// both `char*` and `__FlashStringHelper*` are implicitly converted into `String()`, calling the `operator+(const String& ...);` -// however, here we: -// - do an automatic `reserve(total length)` for the resulting string -// - possibly do rhs.insert(0, ...), when &&rhs capacity could fit both - -String operator +(const char *lhs, const String &rhs); - -inline String operator +(const char *lhs, String &&rhs) { - return std::move(rhs.insert(0, lhs)); -} - -inline String operator +(const __FlashStringHelper *lhs, const String &rhs) { - return reinterpret_cast(lhs) + rhs; -} - -inline String operator +(const __FlashStringHelper *lhs, String &&rhs) { - return std::move(rhs.insert(0, lhs)); -} +class StringSumHelper: public String { + public: + StringSumHelper(const String &s) : + String(s) { + } + StringSumHelper(const char *p) : + String(p) { + } + StringSumHelper(char c) : + String(c) { + } + StringSumHelper(unsigned char num) : + String(num) { + } + StringSumHelper(int num) : + String(num) { + } + StringSumHelper(unsigned int num) : + String(num) { + } + StringSumHelper(long num) : + String(num) { + } + StringSumHelper(unsigned long num) : + String(num) { + } + StringSumHelper(float num) : + String(num) { + } + StringSumHelper(double num) : + String(num) { + } + StringSumHelper(const __FlashStringHelper *s) : + String(s) { + } +}; extern const String emptyString; diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index 1150ba5913..84e3bf8e46 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -545,3 +545,41 @@ TEST_CASE("Replace and string expansion", "[core][String]") REQUIRE(l.length() == strlen(buff)); } } + +TEST_CASE("String chaining", "[core][String]") +{ + const char* chunks[] { + "~12345", + "67890", + "qwertyuiopasdfghjkl", + "zxcvbnm" + }; + + String all; + for (auto* chunk : chunks) { + all += chunk; + } + + // make sure we can chain a combination of things to form a String + REQUIRE((chunks[0] + String(chunks[1]) + F(chunks[2]) + chunks[3]) == all); + REQUIRE((String(chunks[0]) + F(chunks[1]) + F(chunks[2]) + String(chunks[3])) == all); + REQUIRE(('~' + String(&chunks[0][0] + 1) + chunks[1] + String(chunks[2]) + F(chunks[3])) == all); + REQUIRE((String(chunks[0]) + '6' + (&chunks[1][0] + 1) + String(chunks[2]) + F(chunks[3])) == all); + + // these are still invalid (and also cannot compile at all): + // - `F(...)` + `F(...)` + // - `F(...)` + `const char*` + // - `const char*` + `F(...)` + // we need `String()` as either rhs or lhs + + // ensure chaining reuses the buffer + // (internal details...) + { + String tmp(chunks[3]); + tmp.reserve(2 * all.length()); + auto* ptr = tmp.c_str(); + String result("~1" + String(&chunks[0][0] + 2) + F(chunks[1]) + chunks[2] + std::move(tmp)); + REQUIRE(result == all); + REQUIRE(static_cast(result.c_str()) == static_cast(ptr)); + } +} From 6a3fc8679657404cf7a6dd7a375b19503f8d0f4c Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 06:31:33 +0300 Subject: [PATCH 08/16] fixup! fix accidental realloc, add test for operator+ --- cores/esp8266/WString.cpp | 136 ++++++++++++++++--------------- cores/esp8266/WString.h | 163 +++++++++++++++++++++++--------------- 2 files changed, 173 insertions(+), 126 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 07ab99b2cf..e0012af297 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -21,7 +21,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ -#include +#include "Arduino.h" #include "WString.h" #include "stdlib_noniso.h" @@ -50,11 +50,6 @@ String::String(String &&rval) noexcept { move(rval); } -String::String(StringSumHelper &&rval) noexcept { - init(); - move(rval); -} - String::String(unsigned char value, unsigned char base) { init(); char buf[1 + 8 * sizeof(unsigned char)]; @@ -340,84 +335,97 @@ unsigned char String::concat(const __FlashStringHelper *str) { } /*********************************************/ -/* Concatenate */ +/* Insert */ /*********************************************/ -StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(rhs.buffer(), rhs.len())) - a.invalidate(); - return a; -} +String &String::insert(size_t position, const char *other, size_t other_length) { + if (position > length()) { + return *this; + } -StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr) { - StringSumHelper &a = const_cast(lhs); - if (!cstr || !a.concat(cstr, strlen(cstr))) - a.invalidate(); - return a; -} + auto total = length() + other_length; + if (!reserve(total)) + return *this; -StringSumHelper &operator +(const StringSumHelper &lhs, char c) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(c)) - a.invalidate(); - return a; + auto left = length() - position; + setLen(total); + + auto* start = wbuffer() + position; + memmove(start + other_length, start, left); + memmove_P(start, other, other_length); + + return *this; } -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String &String::insert(size_t position, const __FlashStringHelper* other) { + auto* p = reinterpret_cast(other); + return insert(position, p, strlen_P(p)); } -StringSumHelper &operator +(const StringSumHelper &lhs, int num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String &String::insert(size_t position, char other) { + char tmp[2] { other, '\0' }; + return insert(position, tmp, 1); } -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String &String::insert(size_t position, const char *other) { + return insert(position, other, strlen(other)); } -StringSumHelper &operator +(const StringSumHelper &lhs, long num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String &String::insert(size_t position, const String& other) { + return insert(position, other.c_str(), other.length()); } -StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String operator +(const String &lhs, String &&rhs) { + String res; + auto total = lhs.length() + rhs.length(); + if (rhs.capacity() > total) { + rhs.insert(0, lhs); + res = std::move(rhs); + } else { + res.reserve(total); + res += lhs; + res += rhs; + rhs.invalidate(); + } + + return res; } -StringSumHelper &operator +(const StringSumHelper &lhs, float num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String operator +(String&& lhs, String&& rhs) { + String res; + auto total = lhs.length() + rhs.length(); + if (lhs.capacity() > total) { + lhs += rhs; + rhs.invalidate(); + res = std::move(lhs); + } else if (rhs.capacity() > total) { + rhs.insert(0, lhs); + res = std::move(rhs); + } else { + res.reserve(total); + res += lhs; + res += rhs; + lhs.invalidate(); + rhs.invalidate(); + } + + return res; } -StringSumHelper &operator +(const StringSumHelper &lhs, double num) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(num)) - a.invalidate(); - return a; +String operator +(char lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + 1); + res += lhs; + res += rhs; + return res; } -StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs) { - StringSumHelper &a = const_cast(lhs); - if (!a.concat(rhs)) - a.invalidate(); - return a; +String operator +(const char *lhs, const String &rhs) { + String res; + res.reserve(rhs.length() + strlen_P(lhs)); + res += lhs; + res += rhs; + return res; } /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index dcb0982387..3dbf54b141 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -23,14 +23,15 @@ #define String_class_h #ifdef __cplusplus -#include -#include -#include #include -// An inherited class for holding the result of a concatenation. These -// result objects are assumed to be writable by subsequent concatenations. -class StringSumHelper; +#include +#include +#include +#include + +#include +#include // an abstract class used as a means to proide a unique pointer type // but really has no body @@ -38,6 +39,10 @@ class __FlashStringHelper; #define FPSTR(pstr_pointer) (reinterpret_cast(pstr_pointer)) #define F(string_literal) (FPSTR(PSTR(string_literal))) +// support libraries that expect this name to be available +// replace with `using StringSumHelper = String;` in case something wants this constructible +class StringSumHelper; + // The string class class String { // use a function pointer to allow for "if (s)" without the @@ -60,7 +65,6 @@ class String { String(const String &str); String(const __FlashStringHelper *str); String(String &&rval) noexcept; - String(StringSumHelper &&rval) noexcept; explicit String(char c) { sso.buff[0] = c; sso.buff[1] = 0; @@ -100,8 +104,10 @@ class String { String &operator =(const char *cstr); String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; - String &operator =(StringSumHelper &&rval) noexcept { - return operator =((String &&)rval); + String &operator =(char c) { + char buffer[2] { c, '\0' }; + *this = buffer; + return *this; } // concatenate (works w/ built-in types) @@ -132,6 +138,10 @@ class String { concat(cstr); return *this; } + String &operator +=(const __FlashStringHelper *str) { + concat(str); + return *this; + } String &operator +=(char c) { concat(c); return *this; @@ -164,22 +174,6 @@ class String { concat(num); return *this; } - String &operator +=(const __FlashStringHelper *str) { - concat(str); - return *this; - } - - friend StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs); - friend StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr); - friend StringSumHelper &operator +(const StringSumHelper &lhs, char c); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned char num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, int num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned int num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, long num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, float num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, double num); - friend StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs); // comparison (only works w/ Strings and "strings") operator StringIfHelperType() const { @@ -322,6 +316,14 @@ class String { const char *buffer() const { return wbuffer(); } char *wbuffer() const { return isSSO() ? const_cast(sso.buff) : ptr.buff; } // Writable version of buffer + // concatenation is done via non-member functions + // make sure we still have access to internal methods, since we optimize based on capacity of both sides and want to manipulate internal buffers directly + friend String operator +(const String &lhs, String &&rhs); + friend String operator +(String &&lhs, String &&rhs); + friend String operator +(char lhs, String &&rhs); + friend String operator +(const char *lhs, String &&rhs); + friend String operator +(const __FlashStringHelper *lhs, String &&rhs); + protected: void init(void) __attribute__((always_inline)) { sso.buff[0] = 0; @@ -343,48 +345,85 @@ class String { void invalidate(void); unsigned char changeBuffer(unsigned int maxStrLen); - // copy and move + // copy or insert at a specific position String ©(const char *cstr, unsigned int length); String ©(const __FlashStringHelper *pstr, unsigned int length); + + String &insert(size_t position, char); + String &insert(size_t position, const char *); + String &insert(size_t position, const __FlashStringHelper *); + String &insert(size_t position, const char *, size_t length); + String &insert(size_t position, const String &); + + // rvalue helper void move(String &rhs) noexcept; }; -class StringSumHelper: public String { - public: - StringSumHelper(const String &s) : - String(s) { - } - StringSumHelper(const char *p) : - String(p) { - } - StringSumHelper(char c) : - String(c) { - } - StringSumHelper(unsigned char num) : - String(num) { - } - StringSumHelper(int num) : - String(num) { - } - StringSumHelper(unsigned int num) : - String(num) { - } - StringSumHelper(long num) : - String(num) { - } - StringSumHelper(unsigned long num) : - String(num) { - } - StringSumHelper(float num) : - String(num) { - } - StringSumHelper(double num) : - String(num) { - } - StringSumHelper(const __FlashStringHelper *s) : - String(s) { - } -}; +// concatenation (note that it's done using non-method operators to handle both possible type refs) + +inline String operator +(const String &lhs, const String &rhs) { + String res; + res.reserve(lhs.length() + rhs.length()); + res += lhs; + res += rhs; + return res; +} + +inline String operator +(String &&lhs, const String &rhs) { + lhs.concat(rhs); + return std::move(lhs); +} + +String operator +(const String &lhs, String&& rhs); +String operator +(String &&lhs, String &&rhs); + +// concat / += already handles the basic types, so just do that +// TODO: could also do `operator+=` as a templated method +// TODO: explicitly list concat overloads in the enable_if? + +template +inline std::enable_if_t && !std::is_same_v, String>, String> +operator +(const String &lhs, T value) { + String res(lhs); + res += value; + return res; +} + +template +inline std::enable_if_t && !std::is_same_v, String>, String> +operator +(String &&lhs, T value) { + lhs += value; + return std::move(lhs); +} + +// `String(char)` is explicit, but we used to have StringSumHelper silently allowing the following: +// `String x; x = 'a' + String('b') + 'c';` +// For comparison, `std::string(char)` does not exist. However, we are allowed to concatenate `char` like the example above + +String operator +(char lhs, const String &rhs); + +inline String operator +(char lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} + +// both `char*` and `__FlashStringHelper*` are implicitly converted into `String()`, calling the `operator+(const String& ...);` +// however, here we: +// - do an automatic `reserve(total length)` for the resulting string +// - possibly do rhs.insert(0, ...), when &&rhs capacity could fit both + +String operator +(const char *lhs, const String &rhs); + +inline String operator +(const char *lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} + +inline String operator +(const __FlashStringHelper *lhs, const String &rhs) { + return reinterpret_cast(lhs) + rhs; +} + +inline String operator +(const __FlashStringHelper *lhs, String &&rhs) { + return std::move(rhs.insert(0, lhs)); +} extern const String emptyString; From 52c8b37fe832ee6b11ad47c3e698843450a9cb68 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 07:15:43 +0300 Subject: [PATCH 09/16] don't need another branch --- cores/esp8266/WString.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index e0012af297..b9785d9007 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -394,19 +394,13 @@ String operator +(const String &lhs, String &&rhs) { String operator +(String&& lhs, String&& rhs) { String res; auto total = lhs.length() + rhs.length(); - if (lhs.capacity() > total) { - lhs += rhs; - rhs.invalidate(); - res = std::move(lhs); - } else if (rhs.capacity() > total) { + if (rhs.capacity() > total) { rhs.insert(0, lhs); res = std::move(rhs); } else { - res.reserve(total); - res += lhs; - res += rhs; - lhs.invalidate(); + lhs += rhs; rhs.invalidate(); + res = std::move(lhs); } return res; From 5c35ae318ced5ce126826b462b219079035a57ff Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 23 Dec 2020 18:55:57 +0300 Subject: [PATCH 10/16] template +=(String / char* / numbers) and +(String, numbers / char*) --- cores/esp8266/WString.h | 65 ++++++--------------------------- tests/host/core/test_string.cpp | 1 + 2 files changed, 12 insertions(+), 54 deletions(-) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 3dbf54b141..8601f82d77 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -130,48 +130,9 @@ class String { // if there's not enough memory for the concatenated value, the string // will be left unchanged (but this isn't signalled in any way) - String &operator +=(const String &rhs) { - concat(rhs); - return *this; - } - String &operator +=(const char *cstr) { - concat(cstr); - return *this; - } - String &operator +=(const __FlashStringHelper *str) { - concat(str); - return *this; - } - String &operator +=(char c) { - concat(c); - return *this; - } - String &operator +=(unsigned char num) { - concat(num); - return *this; - } - String &operator +=(int num) { - concat(num); - return *this; - } - String &operator +=(unsigned int num) { - concat(num); - return *this; - } - String &operator +=(long num) { - concat(num); - return *this; - } - String &operator +=(unsigned long num) { - concat(num); - return *this; - } - String &operator +=(float num) { - concat(num); - return *this; - } - String &operator +=(double num) { - concat(num); + template + String &operator +=(T &&rhs) { + concat(std::forward(rhs)); return *this; } @@ -370,35 +331,31 @@ inline String operator +(const String &lhs, const String &rhs) { } inline String operator +(String &&lhs, const String &rhs) { - lhs.concat(rhs); + lhs += rhs; return std::move(lhs); } String operator +(const String &lhs, String&& rhs); String operator +(String &&lhs, String &&rhs); -// concat / += already handles the basic types, so just do that -// TODO: could also do `operator+=` as a templated method -// TODO: explicitly list concat overloads in the enable_if? - template -inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(const String &lhs, T value) { +inline std::enable_if_t>, String> +operator +(const String &lhs, T &&value) { String res(lhs); - res += value; + res += std::forward(value); return res; } template -inline std::enable_if_t && !std::is_same_v, String>, String> -operator +(String &&lhs, T value) { - lhs += value; +inline std::enable_if_t>, String> +operator +(String &&lhs, T &&value) { + lhs += std::forward(value); return std::move(lhs); } // `String(char)` is explicit, but we used to have StringSumHelper silently allowing the following: // `String x; x = 'a' + String('b') + 'c';` -// For comparison, `std::string(char)` does not exist. However, we are allowed to concatenate `char` like the example above +// For comparison, `std::string(char)` does not exist. However, we are allowed to chain `char` as both lhs and rhs String operator +(char lhs, const String &rhs); diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index 84e3bf8e46..76110a8ec3 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -561,6 +561,7 @@ TEST_CASE("String chaining", "[core][String]") } // make sure we can chain a combination of things to form a String + REQUIRE((String(chunks[0]) + String(chunks[1]) + String(chunks[2]) + String(chunks[3])) == all); REQUIRE((chunks[0] + String(chunks[1]) + F(chunks[2]) + chunks[3]) == all); REQUIRE((String(chunks[0]) + F(chunks[1]) + F(chunks[2]) + String(chunks[3])) == all); REQUIRE(('~' + String(&chunks[0][0] + 1) + chunks[1] + String(chunks[2]) + F(chunks[3])) == all); From e553e2674f85abf7c06bed09a8dc065f597ecb79 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 6 Jan 2021 05:44:14 +0300 Subject: [PATCH 11/16] nul after moving (isnt mem always zeroed tho?) --- cores/esp8266/WString.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index b9785d9007..01e02ab8d7 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -353,6 +353,7 @@ String &String::insert(size_t position, const char *other, size_t other_length) auto* start = wbuffer() + position; memmove(start + other_length, start, left); memmove_P(start, other, other_length); + wbuffer()[total] = '\0'; return *this; } From 73a28d0d01484f316b3a56bec31beac6420e8d07 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 6 Jan 2021 05:45:24 +0300 Subject: [PATCH 12/16] check if lhs cant keep before switching to rhs --- cores/esp8266/WString.cpp | 2 +- cores/esp8266/WString.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 01e02ab8d7..d5bc0224d1 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -395,7 +395,7 @@ String operator +(const String &lhs, String &&rhs) { String operator +(String&& lhs, String&& rhs) { String res; auto total = lhs.length() + rhs.length(); - if (rhs.capacity() > total) { + if ((total > lhs.capacity()) && (total < rhs.capacity())) { rhs.insert(0, lhs); res = std::move(rhs); } else { diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 8601f82d77..17eba06f20 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -105,7 +105,7 @@ class String { String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; String &operator =(char c) { - char buffer[2] { c, '\0' }; + char buf[2] { c, '\0' }; *this = buffer; return *this; } From fa00fbaf4302af29da29c6194c843f871199a3e7 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 6 Jan 2021 05:46:03 +0300 Subject: [PATCH 13/16] fix String used to store struct data `cannot bind bit-field '...' to 'signed char&' `cannot bind bit-field '...' to 'unssigned char&' noticed in both tasmota and espeasy, where this generates a webpage content from some status struct containing bitfields --- cores/esp8266/WString.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 17eba06f20..6264409356 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -131,8 +131,8 @@ class String { // if there's not enough memory for the concatenated value, the string // will be left unchanged (but this isn't signalled in any way) template - String &operator +=(T &&rhs) { - concat(std::forward(rhs)); + String &operator +=(const T &rhs) { + concat(rhs); return *this; } @@ -335,21 +335,21 @@ inline String operator +(String &&lhs, const String &rhs) { return std::move(lhs); } -String operator +(const String &lhs, String&& rhs); +String operator +(const String &lhs, String &&rhs); String operator +(String &&lhs, String &&rhs); -template -inline std::enable_if_t>, String> -operator +(const String &lhs, T &&value) { +template >>> +inline String operator +(const String &lhs, const T &value) { String res(lhs); - res += std::forward(value); + res += value; return res; } -template -inline std::enable_if_t>, String> -operator +(String &&lhs, T &&value) { - lhs += std::forward(value); +template >>> +inline String operator +(String &&lhs, const T &value) { + lhs += value; return std::move(lhs); } From 8af5a0046ec3b448a1988027bc4f153279cb9e1f Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 6 Jan 2021 05:48:17 +0300 Subject: [PATCH 14/16] style once more --- cores/esp8266/WString.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index d5bc0224d1..0ba14a9d98 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -339,18 +339,18 @@ unsigned char String::concat(const __FlashStringHelper *str) { /*********************************************/ String &String::insert(size_t position, const char *other, size_t other_length) { - if (position > length()) { + if (position > length()) return *this; - } - auto total = length() + other_length; + auto len = length(); + auto total = len + other_length; if (!reserve(total)) return *this; - auto left = length() - position; + auto left = len - position; setLen(total); - auto* start = wbuffer() + position; + auto *start = wbuffer() + position; memmove(start + other_length, start, left); memmove_P(start, other, other_length); wbuffer()[total] = '\0'; @@ -358,8 +358,8 @@ String &String::insert(size_t position, const char *other, size_t other_length) return *this; } -String &String::insert(size_t position, const __FlashStringHelper* other) { - auto* p = reinterpret_cast(other); +String &String::insert(size_t position, const __FlashStringHelper *other) { + auto *p = reinterpret_cast(other); return insert(position, p, strlen_P(p)); } @@ -372,7 +372,7 @@ String &String::insert(size_t position, const char *other) { return insert(position, other, strlen(other)); } -String &String::insert(size_t position, const String& other) { +String &String::insert(size_t position, const String &other) { return insert(position, other.c_str(), other.length()); } @@ -392,7 +392,7 @@ String operator +(const String &lhs, String &&rhs) { return res; } -String operator +(String&& lhs, String&& rhs) { +String operator +(String &&lhs, String &&rhs) { String res; auto total = lhs.length() + rhs.length(); if ((total > lhs.capacity()) && (total < rhs.capacity())) { @@ -417,7 +417,7 @@ String operator +(char lhs, const String &rhs) { String operator +(const char *lhs, const String &rhs) { String res; - res.reserve(rhs.length() + strlen_P(lhs)); + res.reserve(strlen_P(lhs) + rhs.length()); res += lhs; res += rhs; return res; From c57ae6968fc6175546fa9ee157416c46c509c20d Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 6 Jan 2021 05:50:39 +0300 Subject: [PATCH 15/16] typo --- cores/esp8266/WString.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 6264409356..37cdd30ec6 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -105,7 +105,7 @@ class String { String &operator =(const __FlashStringHelper *str); String &operator =(String &&rval) noexcept; String &operator =(char c) { - char buf[2] { c, '\0' }; + char buffer[2] { c, '\0' }; *this = buffer; return *this; } From 32998335c91512c9c0886b766e17977e58297bb9 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sun, 21 Mar 2021 14:05:32 +0300 Subject: [PATCH 16/16] recover 444002180bca8e36b3014eaf5ecf5e690837b440 --- cores/esp8266/WString.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index cd202b4786..93b9c43a59 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -25,6 +25,12 @@ #include "WString.h" #include "stdlib_noniso.h" +#define OOM_STRING_BORDER_DISPLAY 10 +#define OOM_STRING_THRESHOLD_REALLOC_WARN 128 + +#define __STRHELPER(x) #x +#define STR(x) __STRHELPER(x) // stringifier + /*********************************************/ /* Constructors */ /*********************************************/