CesiumGS / cesium-native

Apache License 2.0
438 stars 215 forks source link

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

Open kring opened 1 month ago

kring commented 1 month ago

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.