Fix -Wmaybe-uninitialized warning

[ahans]

gcc 12 warns about this when compiling in release mode (i.e., with -O2):

In file included from /usr/include/c++/12/unordered_map:46,
                 from /usr/include/c++/12/functional:61,
                 from /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/include/rclcpp/context.hpp:19,
                 from /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:15:
In member function ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::iterator std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_erase(size_type, __node_base_ptr, __node_ptr) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::size_type std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_erase(std::true_type, const key_type&) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:2372:15,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::size_type std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::erase(const key_type&) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:973:24,
    inlined from ‘std::unordered_set<_Value, _Hash, _Pred, _Alloc>::size_type std::unordered_set<_Value, _Hash, _Pred, _Alloc>::erase(const key_type&) [with _Value = std::shared_ptr<std::function<void()> >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _Pred = std::equal_to<std::shared_ptr<std::function<void()> > >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >]’ at /usr/include/c++/12/bits/unordered_set.h:546:26,
    inlined from ‘bool rclcpp::Context::remove_shutdown_callback(ShutdownType, const rclcpp::ShutdownCallbackHandle&)’ at /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:443:34:
/usr/include/c++/12/bits/hashtable.h:2330:9: warning: ‘callback_list_ptr’ may be used uninitialized [-Wmaybe-uninitialized]
 2330 |       --_M_element_count;
      |         ^~~~~~~~~~~~~~~~
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp: In member function ‘bool rclcpp::Context::remove_shutdown_callback(ShutdownType, const rclcpp::ShutdownCallbackHandle&)’:
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:425:70: note: ‘callback_list_ptr’ was declared here
  425 |     std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> * callback_list_ptr;
      |                                                                      ^~~~~~~~~~~~~~~~~
In member function ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::__node_ptr std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_begin() const [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::const_iterator std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::begin() const [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:629:16,
    inlined from ‘std::unordered_set<_Value, _Hash, _Pred, _Alloc>::const_iterator std::unordered_set<_Value, _Hash, _Pred, _Alloc>::begin() const [with _Value = std::shared_ptr<std::function<void()> >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _Pred = std::equal_to<std::shared_ptr<std::function<void()> > >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >]’ at /usr/include/c++/12/bits/unordered_set.h:325:26,
    inlined from ‘std::vector<std::function<void()> > rclcpp::Context::get_shutdown_callback(ShutdownType) const’ at /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:479:25:
/usr/include/c++/12/bits/hashtable.h:466:62: warning: ‘callback_list_ptr’ may be used uninitialized [-Wmaybe-uninitialized]
  466 |       { return static_cast<__node_ptr>(_M_before_begin._M_nxt); }
      |                                                              ^
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp: In member function ‘std::vector<std::function<void()> > rclcpp::Context::get_shutdown_callback(ShutdownType) const’:
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:463:70: note: ‘callback_list_ptr’ was declared here
  463 |     std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> * callback_list_ptr;

[clalancette]

While I agree that this is a bug, I don't think that this is the right way to fix this. In particular, if the shutdown_type below ends up not being one of the cases, then we'll just go ahead and dereference the nullptr. I guess that is better in some ways than derferencing random memory, but it is not correct.

I think the proper thing to do here is probably to add a default label to the switch statement below, and probably throw an exception if it is not one of the expected shutdown types.

The same logic goes for below.

[ahans]

If only called with values that are handled by the switch, this is not a bug. But I agree that a more defensive approach is in order. The way the code is now, introducing actual bugs later is easier than it should be. TBH, I only wanted to silence the warning. I'm glad that you're in favor of a better solution.

The enum in the switch statements is only used internally from within context.cpp, and when calling {add,remove,get}_shutdown_callback(), it is always a constant. So this is actually a compile-time parameter. Adding a default label with a throw, i.e., detecting this at runtime, I would not find appropriate. Instead, I made this a compile-time parameter now, so we can static_assert that it has one of the supported values.

Let me know what you think!

[ahans]

I'm not sure if this even needs to be protected by the mutex. I did not pull it out to retain the original behavior. But if it doesn't need to be protected, moving it up would be better.

[fujitatomoya]

yeah, right. callback_handle does not need to be protected by mutex in this function. probably move the mutex down here to protect the race condition for callback_set is better.

[clalancette]

Yeah, we can move it to only protect callback_set. I doubt it makes much of a difference in practice, though.

[ahans]

Not having the RCLCPP_LOCAL here seemed like an oversight.

[fujitatomoya]

good eye 👁️

[clalancette]

Overall, I like the approach here; it takes care of the original issue, and it should be a bit faster (since we aren't doing the switch statement at runtime).

That said, I'm confused by a C++ aspect of this. That is, how can you have template function declaration in the hpp file and the definition in the cpp file? In my understanding, templates generate code, so anyone who wants to call it necessarily needs the code in the header file (not the implementation). How does this work?

[ahans]

That said, I'm confused by a C++ aspect of this. That is, how can you have template function declaration in the hpp file and the definition in the cpp file? In my understanding, templates generate code, so anyone who wants to call it necessarily needs the code in the header file (not the implementation). How does this work?

You need the template definition when you instantiate a template. In the present case, the only instantiations are the calls from the three public functions in context.cpp (add_on_shutdown_callback, remove_on_shutdown_callback, and add_pre_shutdown_callback). Because we're in the same translation unit (same file even), the template definitions are present and the instantiations succeed. Since the helper methods that are now templates are all private, they cannot be called from elsewhere, and so we don't need the template definition in a header (directly or indirectly through an .inl file). Does that make sense?

[ahans]

There's also the pattern of explicit template instantiation that allows you to reduce compile times and improve encapsulation: When you know upfront the types a template will be instantiated with, you can put the template definition in the .cpp file along with explicit instantiations using those types. In the .hpp you only have the template declaration. As long as you only use the templated functions/classes with things that you have explicit instantiations for, this also works for public functions. But as soon as you try to instantiate with something you don't have an explicit instantiation in the .cpp, you will get a linker error. This pattern may have performance penalties, though, because the compiler can't inline as heavily anymore.

However, in the present case none of that matters, since the methods are private. Having the definitions in the header would yield the exact same code, but compilation of thing that #include "context.hpp" would take a tiny bit longer, since that header would have a bit more code to parse. The bigger argument IMHO is that the definition would unnecessarily clutter the header file.

[fujitatomoya]

lgtm with green CI

[fujitatomoya]

good eye 👁️

[fujitatomoya]

yeah, right. callback_handle does not need to be protected by mutex in this function. probably move the mutex down here to protect the race condition for callback_set is better.

[clalancette]

You need the template definition when you instantiate a template. In the present case, the only instantiations are the calls from the three public functions in context.cpp (add_on_shutdown_callback, remove_on_shutdown_callback, and add_pre_shutdown_callback). Because we're in the same translation unit (same file even), the template definitions are present and the instantiations succeed. Since the helper methods that are now templates are all private, they cannot be called from elsewhere, and so we don't need the template definition in a header (directly or indirectly through an .inl file). Does that make sense?

Yep, that makes sense. I figured it was something like that, but I just wanted to check. Thanks for the explanation.

[clalancette]

This looks good to me, except for the one change below. Once we move the mutex in the remove_callback in remove_shutdown_callback, I'll approve and run CI for it.

[ahans]

This looks good to me, except for the one change below. Once we move the mutex in the remove_callback in remove_shutdown_callback, I'll approve and run CI for it.

Alright, pulled that block out of the lambda now!

[clalancette]

This looks good to me. I'm going to fire up CI on it, but I'd appreciate another look from @fujitatomoya before I merge.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows