WString: direct operator overloads instead of StringSumHelper (#7781)

* wip

* huh, turns out String = 'c' did some weird stuff

* style

* allow "blah" + String, 'c' + String and F("...") + String

also, simplify const char* vs. __FlashStringHelper, and just always use _P functions

* shuffle things into .cpp

* trying to fix arduinojson

based on the implementation, we only need to specify that this symbol is a class

* 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

* fixup! fix accidental realloc, add test for operator+

* don't need another branch

* template +=(String / char* / numbers) and +(String, numbers / char*)

* nul after moving (isnt mem always zeroed tho?)

* check if lhs cant keep before switching to rhs

* 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

* style once more

* typo

* recover 444002180bca8e36b3014eaf5ecf5e690837b440

cores/esp8266/StreamString.h

@@ -236,7 +236,6 @@ public:
     StreamString(const String& string): String(string), S2Stream(this) { }
     StreamString(const __FlashStringHelper *str): String(str), S2Stream(this) { }
     StreamString(String&& string): String(string), S2Stream(this) { }
-    StreamString(StringSumHelper&& sum): String(sum), S2Stream(this) { }
 
     explicit StreamString(char c): String(c), S2Stream(this) { }
     explicit StreamString(unsigned char c, unsigned char base = 10): String(c, base), S2Stream(this) { }
@@ -281,13 +280,6 @@ public:
         resetpp();
         return *this;
     }
-
-    StreamString& operator= (StringSumHelper&& rval)
-    {
-        String::operator=(rval);
-        resetpp();
-        return *this;
-    }
 };
 
 #endif // __STREAMSTRING_H

cores/esp8266/WString.cpp

@@ -21,7 +21,7 @@
  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <Arduino.h>
+#include "Arduino.h"
 #include "WString.h"
 #include "stdlib_noniso.h"
 
@@ -56,11 +56,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)];
@@ -390,98 +385,92 @@ unsigned char String::concat(const __FlashStringHelper *str) {
 }
 
 /*********************************************/
-/*  Concatenate                              */
+/*  Insert                                   */
 /*********************************************/
 
-StringSumHelper &operator +(const StringSumHelper &lhs, const String &rhs) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(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;
+
+    auto len = length();
+    auto total = len + other_length;
+    if (!reserve(total))
+        return *this;
+
+    auto left = len - position;
+    setLen(total);
+
+    auto *start = wbuffer() + position;
+    memmove(start + other_length, start, left);
+    memmove_P(start, other, other_length);
+    wbuffer()[total] = '\0';
+
+    return *this;
 }
 
-StringSumHelper &operator +(const StringSumHelper &lhs, const char *cstr) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!cstr || !a.concat(cstr, strlen(cstr)))
-        a.invalidate();
-    return a;
+String &String::insert(size_t position, const __FlashStringHelper *other) {
+    auto *p = reinterpret_cast<const char*>(other);
+    return insert(position, p, strlen_P(p));
 }
 
-StringSumHelper &operator +(const StringSumHelper &lhs, char c) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(c))
-        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 char num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(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, int num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(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 int num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(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, long num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(num))
-        a.invalidate();
-    return a;
+String operator +(String &&lhs, String &&rhs) {
+    String res;
+    auto total = lhs.length() + rhs.length();
+    if ((total > lhs.capacity()) && (total < rhs.capacity())) {
+        rhs.insert(0, lhs);
+        res = std::move(rhs);
+    } else {
+        lhs += rhs;
+        rhs.invalidate();
+        res = std::move(lhs);
+    }
+
+    return res;
 }
 
-StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(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, long long num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(num))
-        a.invalidate();
-    return a;
-}
-
-StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long long num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(num))
-        a.invalidate();
-    return a;
-}
-
-StringSumHelper &operator +(const StringSumHelper &lhs, float num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(num))
-        a.invalidate();
-    return a;
-}
-
-StringSumHelper &operator +(const StringSumHelper &lhs, double num) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(num))
-        a.invalidate();
-    return a;
-}
-
-StringSumHelper &operator +(const StringSumHelper &lhs, const __FlashStringHelper *rhs) {
-    StringSumHelper &a = const_cast<StringSumHelper &>(lhs);
-    if (!a.concat(rhs))
-        a.invalidate();
-    return a;
+String operator +(const char *lhs, const String &rhs) {
+    String res;
+    res.reserve(strlen_P(lhs) + rhs.length());
+    res += lhs;
+    res += rhs;
+    return res;
 }
 
 /*********************************************/

cores/esp8266/WString.h

@@ -23,14 +23,15 @@
 #define String_class_h
 #ifdef __cplusplus
 
-#include <stdlib.h>
-#include <string.h>
-#include <ctype.h>
 #include <pgmspace.h>
 
-// An inherited class for holding the result of a concatenation.  These
-// result objects are assumed to be writable by subsequent concatenations.
-class StringSumHelper;
+#include <cstdlib>
+#include <cstdint>
+#include <cstring>
+#include <cctype>
+
+#include <utility>
+#include <type_traits>
 
 // 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<const __FlashStringHelper *>(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;
@@ -104,8 +108,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)
@@ -130,72 +136,11 @@ 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) {
+        template <typename T>
+        String &operator +=(const T &rhs) {
             concat(rhs);
             return *this;
         }
-        String &operator +=(const char *cstr) {
-            concat(cstr);
-            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 +=(long long num) {
-            concat(num);
-            return *this;
-        }
-        String &operator +=(unsigned long long num) {
-            concat(num);
-            return *this;
-        }
-        String &operator +=(float num) {
-            concat(num);
-            return *this;
-        }
-        String &operator +=(double num) {
-            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, long long num);
-        friend StringSumHelper &operator +(const StringSumHelper &lhs, unsigned long 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 {
@@ -338,6 +283,14 @@ class String {
         const char *buffer() const { return wbuffer(); }
         char *wbuffer() const { return isSSO() ? const_cast<char *>(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;
@@ -359,54 +312,81 @@ class String {
         void invalidate(void);
         unsigned char changeBuffer(unsigned int maxStrLen);
 
-        // copy and move
+        // copy or insert at a specific position
         String &copy(const char *cstr, unsigned int length);
         String &copy(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(long long num) :
-                String(num) {
-        }
-        StringSumHelper(unsigned long 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 += rhs;
+    return std::move(lhs);
+}
+
+String operator +(const String &lhs, String &&rhs);
+String operator +(String &&lhs, String &&rhs);
+
+template <typename T,
+    typename = std::enable_if_t<!std::is_same_v<String, std::decay_t<T>>>>
+inline String operator +(const String &lhs, const T &value) {
+    String res(lhs);
+    res += value;
+    return res;
+}
+
+template <typename T,
+    typename = std::enable_if_t<!std::is_same_v<String, std::decay_t<T>>>>
+inline String operator +(String &&lhs, const 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 chain `char` as both lhs and rhs
+
+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<const char*>(lhs) + rhs;
+}
+
+inline String operator +(const __FlashStringHelper *lhs, String &&rhs) {
+    return std::move(rhs.insert(0, lhs));
+}
 
 extern const String emptyString;
 

tests/host/core/test_string.cpp

@@ -555,3 +555,42 @@ 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((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);
+  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<const void*>(result.c_str()) == static_cast<const void*>(ptr));
+  }
+}