Fix 394 #419
Fix 394 #419
Conversation
|
If the classes ensure that the |
Karsten1987
force-pushed
the
fix-393
branch
3 times, most recently
from
3247a5d
to
a52c05a
Compare
rclcpp_lifecycle/src/state.cpp
Outdated
| auto ret = rcl_lifecycle_state_init( | ||
| state_handle_, rhs.id(), rhs.label().c_str(), &allocator_); | ||
| if (ret != RCL_RET_OK) { | ||
| reset(); |
There was a problem hiding this comment.
Again, state_handle_ has not been zero initialized, so no guarantee that reset can do the right thing.
Same below.
There was a problem hiding this comment.
I don't see how. If the init returns false, it means that the label pointer is null and thus can be freed correctly.
The same with transitions. We call init on it with two nullptr.
Alternatively, we could manually initialize it to null after calling allocate.
copy and assignment operator for transition remove unused const_casts address comments check for null in copy constructor up use init and fini functions from rcl remove unused include
|
CI: |
| state_handle_ = static_cast<rcl_lifecycle_state_t *>( | ||
| allocator_.allocate(sizeof(rcl_lifecycle_state_t), allocator_.state)); | ||
| if (!state_handle_) { | ||
| throw std::runtime_error("failed to allocate memory for rcl_lifecycle_state_t"); |
There was a problem hiding this comment.
nitpick: probably should be std::bad_alloc (http://en.cppreference.com/w/cpp/memory/new/bad_alloc)
(I don't think this needs to be in for this release, just a fyi)
|
|
||
| // we own the handle, so we have to deep-copy the rhs object | ||
| transition_handle_ = static_cast<rcl_lifecycle_transition_t *>( | ||
| allocator_.allocate(sizeof(rcl_lifecycle_transition_t), allocator_.state)); |
There was a problem hiding this comment.
So I noticed that the allocation here's matching deallocation is in the fini function for this struct (same for above rcl struct in State class). While this isn't wrong, it's definitely different from how the rest of the fini functions work. Rather than iterate over the internet, I'm giving @Karsten1987 the ok to merge (since it's not broken, just weird) and tomorrow I'll help him clean up the implementation.
Connects to #394
This is a redo of #395 split up into each individual issue, which is associated to it. This should help the review process.
This can only be merged after #418 has been merged.