Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork130
Make String move constructor move instead of copy.#21
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
CLAassistant commentedApr 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
leg0 commentedApr 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This fixes#146. |
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 had a look at this, the new implementation of themove() method looks good to me, as do the new testcases.
I did leave some comments inline about the other changes you made, please have a look at those.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codecov-commenter commentedApr 21, 2021
Codecov Report
@@ Coverage Diff @@## master #21 +/- ##==========================================- Coverage 96.04% 96.00% -0.04%========================================== Files 13 13 Lines 835 827 -8 ==========================================- Hits 802 794 -8 Misses 33 33
Continue to review full report at Codecov.
|
The move constructor String::String(String&&) and String::operator=(String&&)now perform move instead of copy.Remove String(StringSumHelper&&) constructor because having it makes no sense:String(String&&) takes care of it - you can pass either String&& orStringSumHelper&& to this constructor. StringSumHelper is derived from Stringand has no data members other than those inherited from String. Even if it didhave some extra data members, truncation would have to happen during move, andnormally that is something you don't want.
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.
Looks good to me now, thanks!
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.
Good work! Thank you@leg0 and@matthijskooijman 🚀
The move constructor String::String(String&&) and String::operator=(String&&)now perform move instead of copy.Remove String(StringSumHelper&&) constructor because having it makes no sense:String(String&&) takes care of it - you can pass either String&& orStringSumHelper&& to this constructor. StringSumHelper is derived from Stringand has no data members other than those inherited from String. Even if it didhave some extra data members, truncation would have to happen during move, andnormally that is something you don't want.cherry-pick from:arduino/ArduinoCore-API#21
The move constructor String::String(String&&) and String::operator=(String&&)
now perform move instead of copy.
Remove String(StringSumHelper&&) constructor because having it makes no sense:
String(String&&) takes care of it - you can pass either String&& or
StringSumHelper&& to this constructor. StringSumHelper is derived from String
and has no data members other than those inherited from String. Even if it did
have some extra data members, truncation would have to happen during move, and
normally that is something you don't want.