Make String::move of an invalidated String result in an invalidated String

[sandeepmistry]

Currently the sketch below prints out:

b is false
s is true

However, I expect the output to be:

b is false
s is false

because the blah() function returns an invalidated String (NULL buffer, zero length). However, since the String s; is equivalent to String s(""), s's buffer is realloc(NULL, 0) resulting in a valid buffer.

String s;

void setup() {
  Serial.begin(9600);
  while(!Serial);

  s = blah();

  if (s) {
    Serial.println("s is true");
  } else {
    Serial.println("s is false");
  }
}

String blah() {
  String b((const char*)NULL);

  if (b) {
    Serial.println("b is true");
  } else {
    Serial.println("b is false");
  }

  return b;
}

void loop() {
}

[ArduinoBot]

✅ Build completed.

Please test this code using one of the following:

⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5127-BUILD-573-linux32.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5127-BUILD-573-linux64.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5127-BUILD-573-windows.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5127-BUILD-573-macosx.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5127-BUILD-573-linuxarm.tar.xz

ℹ️ The linuxarm build is still experimental and may not be always available.

[matthijskooijman]

I wonder why this copy code is even here in the first place? Why not just always take over the buffer of rhs? That would be faster, and the only downside is that the buffer pointer for this string changes (but nobody should rely on it anyway)?

[sandeepmistry]

@matthijskooijman good question!

Maybe it's to avoid memory fragmentation?

@PaulStoffregen @damellis @cmaglie do you have any thoughts on this?

[cmaglie]

the only downside is that the buffer pointer for this string changes (but nobody should rely on it anyway)?

there is the c_str() operator and there are PR that pushes for using it outside the string class also for writing :-)

Maybe it's to avoid memory fragmentation?

I guess that's the reason, move is called from the c++11 move-constructor, so it's probably better to keep the original (rvalue) allocation whenever possible to avoid making "holes".