-
-
Notifications
You must be signed in to change notification settings - Fork 348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply String move semantics to reduce memory re-allocations #2119
Conversation
712b7f6
to
ee5c89c
Compare
@mikee47 Maybe you can port also this change from Arduino: esp8266/Arduino@c24109f ? |
// Are we're after the entire contents of this stream? | ||
if(readPos != 0 || maxLen < size) { | ||
// No: do a regular read | ||
return IDataSourceStream::readString(maxLen); | ||
} | ||
|
||
// Move content into the String object | ||
ensureCapacity(size + 1); // Ensure size < capacity | ||
String s; | ||
if(s.setBuffer({buffer, capacity, size})) { | ||
buffer = nullptr; | ||
readPos = 0; | ||
size = 0; | ||
capacity = 0; | ||
} | ||
return s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this behaviour. It's probably correct in 99% of situations, but the safer option might be to add an overload such as size_t readString(String& s);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the safer option might be to add an overload such as size_t readString(String& s);
Got for it as long as the safer option does not impact compatibility, memory usage and speed.
61ff1d9
to
7e63e3c
Compare
@mikee47 Ping me when this PR is ready. |
Provide overrides in `FileStream` and `DataSourceStream` classes. IDataSourceStream::readBytes() calls readMemoryBlock(), then seek(), by default. Classes can override one or both of these methods for more efficient operation. Applications should use whichever is more appropriate. virtualise readString() and update stream position for consistency with `Stream` method **maxLen parameter not optional**
…ve operations Add MemoryDataStream(String&&) constructor: heap allocation required only if String is using SSO mode. Add overloads to various classes to support r-values.
…tor and setString().
7e63e3c
to
1f6a558
Compare
1f6a558
to
741a8ca
Compare
Compiler may choose copy constructors if move constructors not declared noexcept. As per esp8266/Arduino@c24109f ref. - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-move-noexcept - https://rules.sonarsource.com/cpp/RSPEC-5018?search=noexecept - https://clang.llvm.org/extra/clang-tidy/checks/performance-noexcept-move-constructor.html
741a8ca
to
d2c284c
Compare
@slaff OK, I'm done with this now! |
String::getBuffer
andString::setBuffer
to support general move operationsreadString
method forMemoryDataStream