From 184dd3e812fe16c66c5bf3a78626c1a47bb900e8 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 09:55:11 +0100 Subject: [PATCH 1/9] Remove strlen in calls like String::concat(s, strlen(s)) Instead of calling strlen in a dozen places, instead just call String::concat(s), which will then call strlen. This shrinks the code size of these calls significantly, the StringAppendOperator example on the Arduino Uno shrinks by 72 bytes. This change does incur a slight runtime cost, because there is one extra function call. --- api/String.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api/String.cpp b/api/String.cpp index 6f14ae13..6f3e45f6 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -307,49 +307,49 @@ unsigned char String::concat(unsigned char num) { char buf[1 + 3 * sizeof(unsigned char)]; itoa(num, buf, 10); - return concat(buf, strlen(buf)); + return concat(buf); } unsigned char String::concat(int num) { char buf[2 + 3 * sizeof(int)]; itoa(num, buf, 10); - return concat(buf, strlen(buf)); + return concat(buf); } unsigned char String::concat(unsigned int num) { char buf[1 + 3 * sizeof(unsigned int)]; utoa(num, buf, 10); - return concat(buf, strlen(buf)); + return concat(buf); } unsigned char String::concat(long num) { char buf[2 + 3 * sizeof(long)]; ltoa(num, buf, 10); - return concat(buf, strlen(buf)); + return concat(buf); } unsigned char String::concat(unsigned long num) { char buf[1 + 3 * sizeof(unsigned long)]; ultoa(num, buf, 10); - return concat(buf, strlen(buf)); + return concat(buf); } unsigned char String::concat(float num) { char buf[20]; char* string = dtostrf(num, 4, 2, buf); - return concat(string, strlen(string)); + return concat(string); } unsigned char String::concat(double num) { char buf[20]; char* string = dtostrf(num, 4, 2, buf); - return concat(string, strlen(string)); + return concat(string); } unsigned char String::concat(const __FlashStringHelper * str) @@ -378,7 +378,7 @@ StringSumHelper & operator + (const StringSumHelper &lhs, const String &rhs) StringSumHelper & operator + (const StringSumHelper &lhs, const char *cstr) { StringSumHelper &a = const_cast(lhs); - if (!cstr || !a.concat(cstr, strlen(cstr))) a.invalidate(); + if (!cstr || !a.concat(cstr)) a.invalidate(); return a; } From 59dd9950ff797a803fb449983ae9fb3e1c436406 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 09:58:47 +0100 Subject: [PATCH 2/9] Make string concat, copy and move work on non-terminated strings Previously, these methods took a nul-terminated string and appended it to the current buffer. The length argument (or length of the passed String object) was used to allocate memory, but was not used when actually copying the string. This meant that: - If the passed length was too short, or the string passed in was not nul-terminated, the buffer would overflow. - If the string passed in contained embedded nul characters (i.e, among the first length characters), any characters after the embedded nul would not be copied (but len would be updated to indicate they were). In practice, neither of the above would occur, since the length passed is always the known length of the string, usually as returned by strlen. However, to make this method public, and to allow using this concat method to pass in strings that are not nul-terminated, it should be changed to be more robust. This commit changes the method to use memcpy instead of strcpy, copying exactly the number of bytes passed in. For the current calls to this method, which pass a nul-terminated string, without embedded nul characters and a correct length, this behaviour should not change. However, this concat method can now also be used in the two cases mentioned above. Non-nul-terminated strings now work as expected and for strings with embedded newlines the entire string is copied as-is, instead of leaving uninitialized memory after the embedded nul byte. Note that a lot of operations will still only see the string up to the embedded nul byte, but that's not really fixable unless we reimplement functions like strcmp. --- api/String.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/String.cpp b/api/String.cpp index 6f3e45f6..db980897 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -192,7 +192,7 @@ String & String::copy(const char *cstr, unsigned int length) return *this; } len = length; - strcpy(buffer, cstr); + memcpy(buffer, cstr, length); return *this; } @@ -212,7 +212,7 @@ void String::move(String &rhs) { if (buffer) { if (rhs && capacity >= rhs.len) { - strcpy(buffer, rhs.buffer); + memcpy(buffer, rhs.buffer, rhs.len); len = rhs.len; rhs.len = 0; return; @@ -284,7 +284,7 @@ unsigned char String::concat(const char *cstr, unsigned int length) if (!cstr) return 0; if (length == 0) return 1; if (!reserve(newlen)) return 0; - strcpy(buffer + len, cstr); + memcpy(buffer + len, cstr, length); len = newlen; return 1; } From 81a2d51b212c1ea8594065b34de1abd711cdcc42 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 12:27:00 +0100 Subject: [PATCH 3/9] Simplify String::concat(char) Now concat(const char*, unsigned int) no longer requires a nul-terminated string, we can simplify the concat(char) method to just pass the address of the single character instead of having to create buffer with nul-termination. --- api/String.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/String.cpp b/api/String.cpp index db980897..5bf5862a 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -297,10 +297,7 @@ unsigned char String::concat(const char *cstr) unsigned char String::concat(char c) { - char buf[2]; - buf[0] = c; - buf[1] = 0; - return concat(buf, 1); + return concat(&c, 1); } unsigned char String::concat(unsigned char num) From 3b88acac8ee00fc57bc3328e68823aa53088f891 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 09:25:17 +0100 Subject: [PATCH 4/9] Make String::concat(const char *, unsigned int) public This method is useful when receiving data from external sources that pass an explicit length instead of a NUL-terminated string. --- api/String.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/String.h b/api/String.h index d8f943eb..37842379 100644 --- a/api/String.h +++ b/api/String.h @@ -109,6 +109,7 @@ class String // concatenation is considered unsucessful. unsigned char concat(const String &str); unsigned char concat(const char *cstr); + unsigned char concat(const char *cstr, unsigned int length); unsigned char concat(char c); unsigned char concat(unsigned char num); unsigned char concat(int num); @@ -225,7 +226,6 @@ class String void init(void); void invalidate(void); unsigned char changeBuffer(unsigned int maxStrLen); - unsigned char concat(const char *cstr, unsigned int length); // copy and move String & copy(const char *cstr, unsigned int length); From 84314888a8a73b643469c5609beed6d21d1c037a Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 10:37:38 +0100 Subject: [PATCH 5/9] Add String(char *, unsigned) constructor This constructor allows converting a buffer containing a non-nul-terminated string to a String object, by explicitely passing the length. --- api/String.cpp | 6 ++++++ api/String.h | 1 + 2 files changed, 7 insertions(+) diff --git a/api/String.cpp b/api/String.cpp index 5bf5862a..fc8a196e 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -45,6 +45,12 @@ String::String(const char *cstr) if (cstr) copy(cstr, strlen(cstr)); } +String::String(const char *cstr, unsigned int length) +{ + init(); + if (cstr) copy(cstr, length); +} + String::String(const String &value) { init(); diff --git a/api/String.h b/api/String.h index 37842379..c0a8ce2d 100644 --- a/api/String.h +++ b/api/String.h @@ -68,6 +68,7 @@ class String // fails, the string will be marked as invalid (i.e. "if (s)" will // be false). String(const char *cstr = ""); + String(const char *cstr, unsigned int length); String(const String &str); String(const __FlashStringHelper *str); #if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__) From dd236bfd2c62ed083aaa1b1229d71e85fd92f7a9 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 12:01:53 +0100 Subject: [PATCH 6/9] Don't mess with the original in String::substring Before, substring would (temporarily) add a \0 in the original string at the end of the substring, so the substring could be copied into a new String object. However, now that the String::copy() method can work with non-nul-terminated strings (by passing an explicit length), this trick is not needed and we can just call the copy method instead. --- api/String.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/String.cpp b/api/String.cpp index fc8a196e..b67e99c8 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -632,10 +632,7 @@ String String::substring(unsigned int left, unsigned int right) const String out; if (left >= len) return out; if (right > len) right = len; - char temp = buffer[right]; // save the replaced character - buffer[right] = '\0'; - out = buffer + left; // pointer arithmetic - buffer[right] = temp; //restore character + out.copy(buffer + left, right - left); return out; } From eaab14db09073afc20bcf26bd9b7448a6a1309b0 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Mon, 17 Mar 2014 11:24:58 +0100 Subject: [PATCH 7/9] Add String::concat(const uint8_t *, unsigned int) version This just calls the char* version, but allows calling the method with a uint8_t* as well (which is not uncommon for buffers). --- api/String.h | 1 + 1 file changed, 1 insertion(+) diff --git a/api/String.h b/api/String.h index c0a8ce2d..2034c1d2 100644 --- a/api/String.h +++ b/api/String.h @@ -111,6 +111,7 @@ class String unsigned char concat(const String &str); unsigned char concat(const char *cstr); unsigned char concat(const char *cstr, unsigned int length); + unsigned char concat(const uint8_t *cstr, unsigned int length) {return concat((const char*)cstr, length);} unsigned char concat(char c); unsigned char concat(unsigned char num); unsigned char concat(int num); From 0d83f1afc3367037dbde5323c2abd0ae1bd2c583 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Tue, 4 Aug 2015 10:08:09 +0200 Subject: [PATCH 8/9] Add String(const uint8_t *, unsigned int) constructor This allows creating a String from a uint8_t[] or uint8_t* as well, without having to add explicit casts. --- api/String.h | 1 + 1 file changed, 1 insertion(+) diff --git a/api/String.h b/api/String.h index 2034c1d2..a3d0d9e8 100644 --- a/api/String.h +++ b/api/String.h @@ -69,6 +69,7 @@ class String // be false). String(const char *cstr = ""); String(const char *cstr, unsigned int length); + String(const uint8_t *cstr, unsigned int length) : String((const char*)cstr, length) {} String(const String &str); String(const __FlashStringHelper *str); #if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__) From d6fd84fc7e42703a541de9b0837c1f6e78f97bae Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 31 Dec 2020 13:29:04 +0100 Subject: [PATCH 9/9] fixup! Make string concat, copy and move work on non-terminated strings --- api/String.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/String.cpp b/api/String.cpp index b67e99c8..1b6d4b28 100644 --- a/api/String.cpp +++ b/api/String.cpp @@ -199,6 +199,7 @@ String & String::copy(const char *cstr, unsigned int length) } len = length; memcpy(buffer, cstr, length); + buffer[len] = '\0'; return *this; } @@ -220,6 +221,7 @@ void String::move(String &rhs) if (rhs && capacity >= rhs.len) { memcpy(buffer, rhs.buffer, rhs.len); len = rhs.len; + buffer[len] = '\0'; rhs.len = 0; return; } else { @@ -292,6 +294,7 @@ unsigned char String::concat(const char *cstr, unsigned int length) if (!reserve(newlen)) return 0; memcpy(buffer + len, cstr, length); len = newlen; + buffer[len] = '\0'; return 1; }