-
-
Notifications
You must be signed in to change notification settings - Fork 134
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CLA assistant check
All committers have signed the CLA.
76fa259 to
14e923e
Compare
This fixes #146.
Original PR for reference: arduino/Arduino#6261
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 the move() 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.
codecov-commenter
commented
Apr 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&& 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.
e414de7 to
41599a3
Compare
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&& 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. 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.