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

Use shared_ptr in &Engine::AddWorld() fn #30

Merged
merged 6 commits into from
Apr 28, 2020

Conversation

claireyywang
Copy link
Contributor

@claireyywang claireyywang commented Apr 17, 2020

I think it might be a better practice to create shared_ptr instead of creating local object.

@claireyywang claireyywang self-assigned this Apr 17, 2020
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@iche033
Copy link
Contributor

iche033 commented Apr 17, 2020

The changes look fine. Just curious, was this causing a problem? I think the original intention was that the insert call should make a copy of world so therefore it's safe for the local variable to go out of scope.

@claireyywang
Copy link
Contributor Author

claireyywang commented Apr 17, 2020

The changes look fine. Just curious, was this causing a problem? I think the original intention was that the insert call should make a copy of world so therefore it's safe for the local variable to go out of scope.

There wasn't any problem. I've just been reading about C++ and wondered if using shared_ptr would prevent any future issues. If it's good as is, I'd be fine closing this PR. What do you think? @iche033

@iche033
Copy link
Contributor

iche033 commented Apr 17, 2020

ok sounds good. In that case, we could make worlds just store shared ptr? I saw that Entity's children is now using map of shared ptrs now so we could be consistent here

Claire Wang added 4 commits April 20, 2020 14:22
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang
Copy link
Contributor Author

claireyywang commented Apr 27, 2020

@iche033 friendly ping for visibility. I addressed the previous comment. Ready for another round of review whenever works the best for you. Thanks!

@claireyywang claireyywang merged commit 15cb164 into ign-physics2 Apr 28, 2020
@claireyywang claireyywang deleted the claire/tpelib-engine branch April 28, 2020 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants