WString: direct operator overloads instead of StringSumHelper #7781
Conversation
also, simplify const char* vs. __FlashStringHelper, and just always use _P functions
based on the implementation, we only need to specify that this symbol is a class
| inline String operator +(const __FlashStringHelper *lhs, const String &rhs) { | ||
| return reinterpret_cast<const char*>(lhs) + rhs; | ||
| } |
There was a problem hiding this comment.
As to why - the rest of the .cpp, const char* is expected to sometimes be in flash, meaning it is also valid to do PSTR("123") + String(456)
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
|
Just to compare with something more that just fsbrowser, testing letscontrolit/ESPEasy@c530b08 via Original WString.{h,cpp} PR For example, moving from inline to a function |
|
Beware removing inline, there are cases where it is warranted due to potential uses from IRAM code, i. e. : inlining from an IRAM function will make the inlined function IRAM, while inlining from a flash function will make the same inlined function flash code. In the general case, containers that have contiguous memory, such as String, std::string, std::vector, may do a realloc when appending. The realloc operation has very limited functionality from an ISR, because realloc is not interrupt safe. |
`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
mcspr
changed the title
Every inline is still as-is, comment above meant to check the impact of used operations across String-heavy apps. It is varying though, so it's not really measuring anything useful. As-is, did not it already call the flash-only concat? Removing WIP btw, please take a look! |
|
@d-a-v Not a big problem. Only patch required here is for the new long long concat, operator+ code is automatically generated by the template |
d-a-v
added
the
merge-conflict
d-a-v
removed
the
merge-conflict
|
Also sorry for sleeping on this :) On a related note - can we further break Arduino String? e.g.
|
Do you mean "fix" it ? |
Indeed, s/break/break API, but fix to make a better implementation/. Breakage would be with the API though, this this is what is expected to ship with the Arduino original Cores currently: |
As noticed in the #7758 by @jjsuwa-sys3175, StringSumHelper is redundant. This PR reworks
operator+using the modern C++ features.WIP as operators may need some more work tweaking for size, quick test of FSBrowser shows ~200 more bytes in the rom (but, it is not a direct port of the old class, so that was kind of expected). And I also wanted to see what the CI says about it first.
Stringdirectly when overloadingString operator+. I initially tried overloading likeString::operator+(T) &&/String::operator+(T) const, but that did not work as expected. Will need to verify that that actually does not work though, it may have been some other issueconst_cast<StringSumHelper&>when we want to reuse the buffer viaoperator+=String::insert(position, source)for inner methods. This also technically allows to re-use source buffer if capacity is larger than the current buffer, so in the chained+operation it may replace the current bufferString::operator=(char). Right now, with a construct like:C++ decides to cast
,to a StringSumHelper first, since the most obvious constructorString(char)is marked as explicit and requiresx = String(',');instead of the former. This became apparent after building FSBrowser example with the new operators in place, where it may choose any numeric type from the SumHelper class despite String declaring everything as explicit. I very much doubt that was the intended behaviourString &String::operator+=(T&&)forwarding to theString::concat(T)String operator+(String&& / const String&, T&&)forwarding to theString::concat(T)Note that T&& is allowed to be optimized to a basic copy via
-fipa-sra(included in-Os, 2 and 3) when dealing with pod arguments.Any errors show up as
error: call of overloaded ‘concat(some unknown type)’ is ambiguous<c...>variants. Those are not real C-lib headers anyway with the current G++StringSumHelpername is still present b/c at least ArduinoJson expects it for template specializations