Skip to content

Fix move constructor and operator= in String. #6261

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix move constructor and operator= in String. #6261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2da21ad
Fix move constructor and operator= in String.
leg0 May 5, 2017
a30f73b
Remove redundant null checks.
leg0 Jan 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 21 additions & 31 deletions hardware/arduino/avr/cores/arduino/WString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,15 @@ String::String(const __FlashStringHelper *pstr)
*this = pstr;
}

#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
String::String(String &&rval)
: buffer(rval.buffer)
, capacity(rval.capacity)
, len(rval.len)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really these should all have a call to std::move, but of course std::move is not available on the arduino.

Perhaps it would be sensible to provide std::move and std::forward if arduino is going to start using rvalue references internally

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if std::move was available it would not make much sense to use it here - all it does is to cast is argument to rvalue reference. For integral types and pointers move construction is same as copy construction.

I agree that it would be useful to have move and forward available in lib.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess having std::move on all these assignments would allow move semantics / optimizations on each of the fields, but since we know these are just a pointer and two integers, it should not make any difference here. So I guess the code is good as the PR proposes it.

{
init();
move(rval);
}
String::String(StringSumHelper &&rval)
{
init();
move(rval);
rval.buffer = NULL;
rval.capacity = 0;
rval.len = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for these last two lines - all that matters is that buffer is NULL to prevent rval::~String() from freeing it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to c++ standard, object needs to be in valid state after being moved from. Having null buffer and non-zero capacity and /or length does not seem like a valid state to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the state sane is probably a good idea, so I'd suggest leaving these in.

}
#endif

Expand Down Expand Up @@ -189,25 +188,22 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length)
return *this;
}

#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
void String::move(String &rhs)
{
if (buffer) {
if (rhs && capacity >= rhs.len) {
strcpy(buffer, rhs.buffer);
len = rhs.len;
rhs.len = 0;
return;
} else {
if (this != &rhs)
{
if (buffer != NULL)
free(buffer);
Copy link

@BillyDonahue BillyDonahue Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The if (buffer != NULL) is redundant since free(nullptr) is defined to be a safe no-op.
http://www.cplusplus.com/reference/cstdlib/free/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Removed the null checks before free.

}

buffer = rhs.buffer;
len = rhs.len;
capacity = rhs.capacity;

rhs.buffer = NULL;
rhs.len = 0;
rhs.capacity = 0;
}
buffer = rhs.buffer;
capacity = rhs.capacity;
len = rhs.len;
rhs.buffer = NULL;
rhs.capacity = 0;
rhs.len = 0;
}
#endif

Expand All @@ -221,16 +217,10 @@ String & String::operator = (const String &rhs)
return *this;
}

#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
String & String::operator = (String &&rval)
{
if (this != &rval) move(rval);
return *this;
}

String & String::operator = (StringSumHelper &&rval)
{
if (this != &rval) move(rval);
move(rval);
return *this;
}
#endif
Expand Down
12 changes: 7 additions & 5 deletions hardware/arduino/avr/cores/arduino/WString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ class __FlashStringHelper;
// result objects are assumed to be writable by subsequent concatenations.
class StringSumHelper;

#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#define STRING_HAS_MOVE 1
#endif

// The string class
class String
{
Expand All @@ -59,9 +63,8 @@ class String
String(const char *cstr = "");
String(const String &str);
String(const __FlashStringHelper *str);
#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
String(String &&rval);
String(StringSumHelper &&rval);
#endif
explicit String(char c);
explicit String(unsigned char, unsigned char base=10);
Expand All @@ -86,9 +89,8 @@ class String
String & operator = (const String &rhs);
String & operator = (const char *cstr);
String & operator = (const __FlashStringHelper *str);
#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
String & operator = (String &&rval);
String & operator = (StringSumHelper &&rval);
#endif

// concatenate (works w/ built-in types)
Expand Down Expand Up @@ -205,7 +207,7 @@ class String
// copy and move
String & copy(const char *cstr, unsigned int length);
String & copy(const __FlashStringHelper *pstr, unsigned int length);
#if __cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X__)
#if STRING_HAS_MOVE
void move(String &rhs);
#endif
};
Expand Down