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

Small fixes needed for focal support. #1704

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Conversation

wohe
Copy link
Member

@wohe wohe commented Jun 16, 2020

Signed-off-by: Wolfgang Hess [email protected]

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@ojura
Copy link
Contributor

ojura commented Jun 17, 2020

Wait, you are allowed to return by value an lvalue unique_ptr without moving from it? I remember seeing somewhere that in general std::move on return is a bad thing because it prevents the compiler to do copy elision. But I thought it was necessary with non-copyable types like unique_ptr.

@wohe
Copy link
Member Author

wohe commented Jun 17, 2020

@ojura See https://en.cppreference.com/w/cpp/language/return or https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stackoverflow: Removing the std::move here does not prevent moving.

@wohe wohe requested a review from feuerste June 17, 2020 07:43
Copy link

@feuerste feuerste left a comment

Choose a reason for hiding this comment

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

LGTM

@wohe wohe merged commit e8c1d84 into cartographer-project:master Jun 17, 2020
valgur pushed a commit to valgur/cartographer that referenced this pull request Jun 11, 2022
valgur pushed a commit to valgur/cartographer that referenced this pull request Jun 13, 2022
@anchuanxu
Copy link

thanks!

farhanmustar pushed a commit to dfautomation/cartographer that referenced this pull request Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants