-
Notifications
You must be signed in to change notification settings - Fork 56
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
type trait rcpputils::is_pointer<T>` #19
Conversation
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks for the updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too! Let's merge after green CI.
#include "rcpputils/pointer_traits.hpp" | ||
|
||
TEST(TestPointerTraits, is_pointer) { | ||
auto ptr = new int(13); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should delete
this at the end of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry noticed this after approving and green CI, and we don't run ASan/TSan jobs in CI so this won't get flagged in that either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, you're absolutely right!
Addressed in 49c52d8
|
||
TEST(TestPointerTraits, is_pointer) { | ||
auto ptr = new int(13); | ||
const auto * const cptrc = new int(13); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is also new
allocated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, but this was passed into a shared pointer object and thus deallocated
To stay consistent, I've also addressed this in 49c52d8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right! Thanks for making it more obvious though :)
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
260c3a4
to
49c52d8
Compare
Moving the PR ros2/rclcpp#817 over to
rcpputils
.I hopefully addressed previous feedback about adding documentation to it. Please let me know if the documentation is sufficient or if more details should be provided.
Signed-off-by: Karsten Knese karsten@openrobotics.org