mempool: apply rule of 5 to epochguard.h, fix compiler warnings #22481

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:epochguard-rule-of-three changing 1 files +10 −0
  1. jonatack commented at 0:06 am on July 18, 2021: member

    Apply the rule of five to src/util/epochguard.h::{Epoch, Marker} for safety, which also nicely fixes the -Wdeprecated-copy compiler warnings with Clang 13.

    References:

     0In file included from policy/rbf.cpp:5:
 1In file included from ./policy/rbf.h:8:
 2In file included from ./txmempool.h:24:
 3./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
 4        Marker& operator=(const Marker&) = delete;
 5                ^
 6./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
 7class CTxMemPoolEntry
 8      ^
 9policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
10    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());

     0In file included from txmempool.cpp:6:
 1In file included from ./txmempool.h:24:
 2./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
 3        Marker& operator=(const Marker&) = delete;
 4                ^
 5./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
 6class CTxMemPoolEntry
 7      ^
 8/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
 9        { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
10                             ^
11/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
12          __a.construct(__p, std::forward<_Args>(__args)...);
13              ^
14/usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
15    node_alloc_traits::construct(
16                       ^
17/usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here
18      final().construct_value(x,v);
19              ^
20/usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
21    final_node_type* res=super::insert_(v,x,variant);
22                                ^
23/usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
24    final_node_type* res=super::insert_(v,x,variant);
25                                ^
26/usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
27    return insert_(v,detail::lvalue_tag());
28           ^
29/usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here
30    {return final().insert_(x);}
31                    ^
32/usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>::final_insert_' requested here
33    std::pair<final_node_type*,bool> p=this->final_insert_(x);
34                                             ^
35txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here
36    indexed_transaction_set::iterator newit = mapTx.insert(entry).first;

     0In file included from test/fuzz/policy_estimator.cpp:9:
 1In file included from ./test/fuzz/util.h:27:
 2In file included from ./txmempool.h:24:
 3./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
 4        Marker& operator=(const Marker&) = delete;
 5                ^
 6./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
 7class CTxMemPoolEntry
 8      ^
 9/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
10        { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
11                             ^
12/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
13          __a.construct(__p, std::forward<_Args>(__args)...);
14              ^
15/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
16            _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
17                           ^
18/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::emplace_back<CTxMemPoolEntry>' requested here
19      { emplace_back(std::move(__x)); }
20        ^
21test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
22                    mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
  2. DrahtBot added the label Mempool on Jul 18, 2021
  3. DrahtBot added the label Utils/log/libs on Jul 18, 2021
  4. JeremyRubin commented at 4:46 am on July 18, 2021: contributor
    concept ack; i think you missed one of the defs you were planning to do.
  5. jonatack commented at 7:35 am on July 18, 2021: member
    Thanks @JeremyRubin. I defined the copy constructor and the default constructor; the copy assignment operator is already defined. The move constructor and the move assignment operator (rule of 5) aren’t needed for now, though they can be added.
  6. vasild approved
  7. vasild commented at 10:22 am on July 20, 2021: member

    ACK 8fc52af31199496d86a1b300f25826e89322d779

    Thanks for fixing this! Now I can remove -Wno-deprecated-copy from my build environment.

    I would go with the rule of 5 for both Epoch and Marker. If nobody is using the move assignment operator (constructor), then now is a good time to = delete; it since the intention seems to be // only allow modification via Epoch member functions. Why virtual Epoch destructor in the OP?

    cc @ajtowns the code originated in https://github.com/bitcoin/bitcoin/pull/18017

  8. mempool: apply rule of 5 to epochguard.h, fix compiler warnings 7b3a20b260
  9. jonatack force-pushed on Jul 20, 2021
  10. jonatack renamed this:
    mempool: apply rule of 3 to epochguard.h, fix compiler warnings
    mempool: apply rule of 5 to epochguard.h, fix compiler warnings
    on Jul 20, 2021
  11. jonatack commented at 12:25 pm on July 20, 2021: member
    @vasild thanks! Done; upgraded from the minimum rule of 3 change needed to quiet the compiler, to rule of five.
  12. vasild approved
  13. vasild commented at 9:49 am on July 22, 2021: member
    ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
  14. laanwj commented at 12:53 pm on July 22, 2021: member

    i think we should at least implement the rule of 7 here !!!

    Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc

  15. laanwj merged this on Jul 22, 2021
  16. laanwj closed this on Jul 22, 2021
  17. jonatack deleted the branch on Jul 22, 2021
  18. in src/util/epochguard.h:44 in 7b3a20b260 
    39@@ -40,6 +40,9 @@ class LOCKABLE Epoch
40     Epoch() = default;
41     Epoch(const Epoch&) = delete;
42     Epoch& operator=(const Epoch&) = delete;
43+    Epoch(Epoch&&) = delete;
44+    Epoch& operator=(Epoch&&) = delete;
    MarcoFalke commented at 3:09 pm on July 22, 2021:

    The && constructors should already be deleted, no?

    the presence of a user-defined destructor, copy-constructor, or copy-assignment operator prevents implicit definition of the move constructor and the move assignment operator

    From the page you linked (https://en.cppreference.com/w/cpp/language/rule_of_three#Rule_of_five)

    And quoting from your error message this should be the case: “… has a user-declared copy assignment operator”.

    MarcoFalke commented at 3:18 pm on July 22, 2021:
    Ah ok, the other pages you linked to seem to always prefer all 5 (or zero)
  19. in src/util/epochguard.h:62 in 7b3a20b260 
    53@@ -51,6 +54,13 @@ class LOCKABLE Epoch
54         // only allow modification via Epoch member functions
55         friend class Epoch;
56         Marker& operator=(const Marker&) = delete;
57+
58+    public:
59+        Marker() = default;
60+        Marker(const Marker&) = default;
61+        Marker(Marker&&) = delete;
62+        Marker& operator=(Marker&&) = delete;
    MarcoFalke commented at 3:09 pm on July 22, 2021:
    Same
  20. sidhujag referenced this in commit 50d5494282 on Jul 23, 2021
  21. practicalswift commented at 9:09 pm on July 24, 2021: contributor
    Suggested follow-up: could perhaps add a developer note about “rule of […]” to make expectations clear for new code and new contributors? :)
  22. vasild commented at 7:59 am on July 26, 2021: member
    That is just one of many C++ good practices.
  23. Fabcien referenced this in commit 118a1c1fee on Apr 8, 2022
  24. DrahtBot locked this on Aug 18, 2022


jonatack JeremyRubin vasild laanwj MarcoFalke practicalswift

Labels
Mempool Utils/log/libs

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: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me