gui: change combiner for signals to optional_last_value #19256

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:boost_optional_last_value changing 2 files +6 −6
  1. fanquake commented at 5:31 AM on June 12, 2020: member

    optional_last_value, which does not throw, has replaced last_value as Boosts default combiner. Besides being better supported, it also doesn't trigger gcc's -Wmaybe-unitialized warning, presumably because exceptions no longer bubble-up out of signals:

    In file included from ui_interface.cpp:9:
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
             if(value) return value.get();
                                        ^
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
             optional<T> value;
                         ^~~~~
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
             if(value) return value.get();
                                        ^
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
             optional<T> value;
                         ^~~~~
    

    The change in default happened in Boost 1.39.0 (along with the introduction of the Signals2 library.

    More information is also available here https://www.boost.org/doc/libs/1_73_0/doc/html/signals2/rationale.html#id-1.3.36.9.4:

    The default combiner for Boost.Signals2 has changed from the last_value combiner used by default in the original Boost.Signals library. This is because last_value requires that at least 1 slot be connected to the signal when it is invoked (except for the last_value<void> specialization). In a multi-threaded environment where signal invocations and slot connections and disconnections may be happening concurrently, it is difficult to fulfill this requirement. When using optional_last_value, there is no requirement for slots to be connected when a signal is invoked, since in that case the combiner may simply return an empty boost::optional.

  2. fanquake added the label Refactoring on Jun 12, 2020
  3. fanquake force-pushed on Jun 12, 2020
  4. hebasto commented at 7:57 AM on June 12, 2020: member

    Which GCC version should I use to get those warnings?

  5. fanquake commented at 8:12 AM on June 12, 2020: member

    Which GCC version should I use to get those warnings?

    I'd assume any recent GCC will produce them. I was testing with 8.3.0:

    gcc --version               
    gcc (Debian 8.3.0-6) 8.3.0
    
    ./autogen.sh
    ./configure --prefix=/bitcoin/depends/x86_64-pc-linux-gnu/ CXXFLAGS="-Wuninitialized" 
    ...
      CXX           = g++ -m64 -std=c++11
      CXXFLAGS      =   -fstack-reuse=none -Wstack-protector -fstack-protector-all     -pipe -O2 -Wuninitialized -fno-extended-identifiers
    ...
    make src/bitcoind -j6
    ...
      CXX      libbitcoin_common_a-core_write.o
    In file included from ui_interface.cpp:9:
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
             if(value) return value.get();
                                        ^
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
             optional<T> value;
                         ^~~~~
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp: In member function 'boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::result_type boost::signals2::detail::signal_impl<R(Args ...), Combiner, Group, GroupCompare, SlotFunction, ExtendedSlotFunction, Mutex>::operator()(Args ...) [with Combiner = boost::signals2::last_value<bool>; Group = int; GroupCompare = std::less<int>; SlotFunction = boost::function<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; ExtendedSlotFunction = boost::function<bool(const boost::signals2::connection&, const bilingual_str&, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, unsigned int)>; Mutex = boost::signals2::mutex; R = bool; Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int}]':
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
             if(value) return value.get();
                                        ^
    /bitcoin/depends/x86_64-pc-linux-gnu/share/../include/boost/signals2/last_value.hpp:43:21: note: '*((void*)& value +1)' was declared here
             optional<T> value;
                         ^~~~~
      CXX      libbitcoin_common_a-key.o
    
  6. laanwj commented at 10:42 AM on June 12, 2020: member

    I think this change is fine. I'd prefer to call it what it is though "Change combiner for GUI signals to optional_last_value". This does have a subtle change of functionality: namely that not registering any GUI signals is no longer a run-time error and message boxes could potentially end up ignored when no one registers a handler. Fixing the warning is only a by-effect.

  7. fanquake force-pushed on Jun 12, 2020
  8. fanquake renamed this:
    refactor: fix Boost signals2 -Wmaybe-uninitialized warning
    gui: change combiner for signals to optional_last_value
    on Jun 12, 2020
  9. fanquake removed the label Refactoring on Jun 12, 2020
  10. fanquake added the label GUI on Jun 12, 2020
  11. fanquake commented at 12:25 PM on June 12, 2020: member

    I'd prefer to call it what it is though

    I've updated the commit message.

  12. gui: change combiner for signals to optional_last_value
    optional_last_value, which does not throw, has replaced optional_value as
    boost's default combiner. Besides being better supported, it also doesn't
    trigger gcc's -Wmaybe-unitialized warning, presumably because exceptions no
    longer bubble-up out of signals:
    
    ```bash
    boost/signals2/last_value.hpp:54:36: warning: '*((void*)& value +1)' may be used uninitialized in this function [-Wmaybe-uninitialized]
    	if(value) return value.get();
    ```
    
    The change in default happened in Boost 1.39.0 (along with the
    introduction of the signals 2 library. More information is available here:
    
    https://www.boost.org/doc/libs/1_73_0/doc/html/signals2/rationale.html#id-1.3.36.9.4
    
    and here:
    
    https://www.boost.org/doc/libs/1_73_0/doc/html/boost/signals2/optional_last_value.html
    
    Co-authored-by: fanquake <fanquake@gmail.com>
    f1a0314c53
  13. fanquake force-pushed on Jul 1, 2020
  14. laanwj commented at 2:04 PM on July 1, 2020: member

    ACK f1a0314c537791f202dfb7c1209f0e04ba7988c3

  15. laanwj merged this on Jul 1, 2020
  16. laanwj closed this on Jul 1, 2020

  17. fanquake deleted the branch on Jul 2, 2020
  18. in src/node/ui_interface.cpp:46 in f1a0314c53
      41 | @@ -42,8 +42,8 @@ ADD_SIGNALS_IMPL_WRAPPER(NotifyBlockTip);
      42 |  ADD_SIGNALS_IMPL_WRAPPER(NotifyHeaderTip);
      43 |  ADD_SIGNALS_IMPL_WRAPPER(BannedListChanged);
      44 |  
      45 | -bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, caption, style); }
      46 | -bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, caption, style); }
      47 | +bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, caption, style).value_or(false);}
      48 | +bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, caption, style).value_or(false);}
    


    MarcoFalke commented at 6:16 PM on July 13, 2020:
    node/ui_interface.cpp: In member function ‘bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, const string&, unsigned int)’:
    node/ui_interface.cpp:45:193: error: ‘boost::signals2::signal<bool(const bilingual_str&, const std::basic_string<char>&, unsigned int), boost::signals2::optional_last_value<bool> >::result_type’ has no member named ‘value_or’
     bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, caption, style).value_or(false);}
                                                                                                                                                                                                     ^
    node/ui_interface.cpp: In member function ‘bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const string&, const string&, unsigned int)’:
    node/ui_interface.cpp:46:258: error: ‘boost::signals2::signal<bool(const bilingual_str&, const std::basic_string<char>&, const std::basic_string<char>&, unsigned int), boost::signals2::optional_last_value<bool> >::result_type’ has no member named ‘value_or’
    

    MarcoFalke commented at 6:16 PM on July 13, 2020:

    boost 1.53


    MarcoFalke commented at 6:33 PM on July 13, 2020:

    Same with boost 1.55

    #  make 
    Making all in src
    make[1]: Entering directory '/bitcoin/src'
    make[2]: Entering directory '/bitcoin/src'
    make[3]: Entering directory '/bitcoin'
    make[3]: Leaving directory '/bitcoin'
      CXXLD    libbitcoinconsensus.la
      CXX      node/libbitcoin_server_a-ui_interface.o
    node/ui_interface.cpp: In member function 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, const string&, unsigned int)':
    node/ui_interface.cpp:45:193: error: 'boost::signals2::signal<bool(const bilingual_str&, const std::basic_string<char>&, unsigned int), boost::signals2::optional_last_value<bool> >::result_type' has no member named 'value_or'
     bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, caption, style).value_or(false);}
                                                                                                                                                                                                     ^
    node/ui_interface.cpp: In member function 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const string&, const string&, unsigned int)':
    node/ui_interface.cpp:46:258: error: 'boost::signals2::signal<bool(const bilingual_str&, const std::basic_string<char>&, const std::basic_string<char>&, unsigned int), boost::signals2::optional_last_value<bool> >::result_type' has no member named 'value_or'
     bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, caption, style).value_or(false);}
                                                                                                                                                                                                                                                                      ^
    node/ui_interface.cpp: In member function 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, const string&, unsigned int)':
    node/ui_interface.cpp:45:209: warning: control reaches end of non-void function [-Wreturn-type]
     bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, caption, style).value_or(false);}
                                                                                                                                                                                                                     ^
    node/ui_interface.cpp: In member function 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const string&, const string&, unsigned int)':
    node/ui_interface.cpp:46:274: warning: control reaches end of non-void function [-Wreturn-type]
     bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, const std::string& caption, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, caption, style).value_or(false);}
                                                                                                                                                                                                                                                                                      ^
    Makefile:10774: recipe for target 'node/libbitcoin_server_a-ui_interface.o' failed
    make[2]: *** [node/libbitcoin_server_a-ui_interface.o] Error 1
    make[2]: Leaving directory '/bitcoin/src'
    Makefile:17685: recipe for target 'all-recursive' failed
    make[1]: *** [all-recursive] Error 1
    make[1]: Leaving directory '/bitcoin/src'
    Makefile:775: recipe for target 'all-recursive' failed
    make: *** [all-recursive] Error 1
    
  19. Fabcien referenced this in commit 4728a3d789 on Aug 28, 2021
  20. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me