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

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

Open
kring opened this issue Oct 3, 2024 · 0 comments
Open
Labels
good first issue Good for newcomers quality Improve code quality or encourage developer success

Comments

@kring
Copy link
Member

kring commented Oct 3, 2024

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 kring added good first issue Good for newcomers quality Improve code quality or encourage developer success labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers quality Improve code quality or encourage developer success
Projects
None yet
Development

No branches or pull requests

1 participant