Skip to content
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

Make String move constructor move instead of copy. #21

Merged
merged 2 commits into from
May 25, 2021

Conversation

leg0
Copy link
Contributor

@leg0 leg0 commented Nov 15, 2018

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.

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@leg0
Copy link
Contributor Author

leg0 commented Apr 20, 2021

This fixes #146.
Original PR for reference: arduino/Arduino#6261

Copy link
Collaborator

@matthijskooijman matthijskooijman left a 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.

api/String.cpp Show resolved Hide resolved
api/String.cpp Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #21 (e414de7) into master (e2d2f20) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
api/String.h 90.00% <ø> (ø)
api/String.cpp 97.65% <100.00%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2d2f20...e414de7. Read the comment docs.

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.
Copy link
Collaborator

@matthijskooijman matthijskooijman left a 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!

Copy link
Contributor

@aentinger aentinger left a 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 🚀

@aentinger aentinger merged commit 42f8e11 into arduino:master May 25, 2021
@leg0 leg0 deleted the proper_string_move branch May 25, 2021 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants