cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

Add our own `std::move` that fails to compile when used on a const object

Open kring opened this issue 1 year ago • 0 comments

It's really easy to accidentally try to move an object that can't actually be moved because it's const. For example:

std::string value = "I like to move it move it";

auto lambda = [capturedValue= std::move(value)]() {
  callSomething(std::move(capturedValue));
};

lambda();

Seems legit at a glance, right? Nope, the std::move(capturedValue) does nothing at all, because capturedValue is const, despite the word const not appearing anywhere. So if callSomething has two overloads, one that takes a const std::string& and the other a std::string&&, this will end up calling the const std::string& one. And so we'll get a copy, which can have pretty severe performance implications in some cases.

The solution is pretty subtle:

std::string value = "I like to move it move it";

auto lambda = [capturedValue= std::move(value)]() mutable {
  callSomething(std::move(capturedValue));
};

lambda();

The only change in this new version, which will work as expected, is that the lambda has been declared mutable.

Unreal Engine has a function called MoveTemp that helps avoid this class of problems. It's just like std::move, except it also uses a static_assert to cause a compiler error if given a const reference. We should add something similar to cesium-native (perhaps CesiumUtility::move?), and use it everywhere. There's a chance it will immediately flag some copies we didn't realize we were making.

I suspect something like clang-tidy can flag this, too, so that's another option.

kring avatar Oct 03 '24 08:10 kring