Replace boost::bind with std::bind
src/rpc/server.cpp, replace std::transform with simple loop.src/validation.cpp, store the boost::signals2::connection object and use it to disconnect.src/validationinterface.cpp, use 2 map to store the boost::signals2::scoped_connection object.
Concept ACK
After this boost::bind will still be used by server, scheduler and validation (and associated tests). @ken2812221 Is there a particular reason you haven’t removed that usage?
Concept ACK, but please remove using namespace std::placeholders;. Reference _N using the fully specified std::placeholders::_N.
See developer notes:
- Don’t import anything into the global namespace (
using namespace ...). Use fully specified types such asstd::string.
After this boost::bind will still be used by server, scheduler and validation (and associated tests). Is there a particular reason you haven’t removed that usage?
Those instances cannot be replaced simply boost -> std. I’ll try to get rid of those.
please remove
using namespace std::placeholders;
Will do
58@@ -57,6 +59,8 @@
59 #include <QUrlQuery>
60 #include <QVBoxLayout>
61
62+using namespace std::placeholders;
29@@ -28,6 +30,8 @@
30 #include <QDebug>
31 #include <QTimer>
32
33+using namespace std::placeholders;
25@@ -24,6 +26,8 @@
26 #include <QPainter>
27 #include <QRadialGradient>
28
29+using namespace std::placeholders;
28@@ -27,6 +29,8 @@
29 #include <QIcon>
30 #include <QList>
31
32+using namespace std::placeholders;
28@@ -27,6 +29,7 @@
29 #include <QSet>
30 #include <QTimer>
31
32+using namespace std::placeholders;
27@@ -27,6 +28,8 @@
28 #include <event2/event.h>
29 #include <event2/thread.h>
30
31+using namespace std::placeholders;
using namespace std::placeholders here, since the alternative would be extremely verbose.
std::bind (modulo the few places where that is not technically possible in C++11), no? :-)
using namespace std::placeholders; is acceptable, I will wait for more comments from other people and decide what to do.
std::bind too :-)
506@@ -508,9 +507,10 @@ std::vector<std::string> CRPCTable::listCommands() const
507 std::vector<std::string> commandList;
508 typedef std::map<std::string, const CRPCCommand*> commandMap;
using place = namespace std::placeholders; as an alternative? The calls would be relatively less verbose and would read well, e.g. std::bind(ThreadSafeMessageBox, this, place::_1, place::_2, place::_3). It would avoid polluting the global namespace, and would make the connection more explicit from source to reference.
99+ connections.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, _1));
100+ connections.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, _1));
101+ connections.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2));
102+ connections.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
103+ connections.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));
104+ g_signals.m_wallet_connections.emplace(pwalletIn, connections);
std::move connections
125+ connections.BlockDisconnected.disconnect();
126+ connections.TransactionRemovedFromMempool.disconnect();
127+ connections.ChainStateFlushed.disconnect();
128+ connections.Broadcast.disconnect();
129+ connections.BlockChecked.disconnect();
130+ connections.NewPoWValidBlock.disconnect();
ValidationInterfaceConnections where it would be more clear whether the method was comprehensive in its disconnecting.
98+ connections.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1));
99+ connections.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, _1));
100+ connections.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, _1));
101+ connections.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2));
102+ connections.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
103+ connections.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));
ValidationInterfaceConnections
5@@ -6,10 +6,13 @@
6 #ifndef BITCOIN_VALIDATIONINTERFACE_H
7 #define BITCOIN_VALIDATIONINTERFACE_H
8
9+#include <boost/signals2/signal.hpp>
10+
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
std::bind and lambda in the first comment for the convenience of the reviewers? It might not be obvious to everyone why lambdas should be preferred over std::bind everywhere.
@MarcoFalke Stephan T. Lavavej (standard library maintainer Visual C++) sums up the std::bind issues/gotchas here: https://www.youtube.com/watch?v=zt7ThwVfap0&t=1721
His recommendations:
- Avoid using
bind()- Use lambdas, especially generic lambdas
bind(): good idea in 2005, bad idea in 2015
Fun quote from the video:
“I’ve seen both Herb Sutter and Scott Meyers confused by
bind. These are very very smart people, butbindis even more evil than they’re able to handle. I was confused bybindwhile maintainingbind…”
252- m_handler_notify_network_active_changed = m_node.handleNotifyNetworkActiveChanged(boost::bind(NotifyNetworkActiveChanged, this, _1));
253- m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(boost::bind(NotifyAlertChanged, this));
254- m_handler_banned_list_changed = m_node.handleBannedListChanged(boost::bind(BannedListChanged, this));
255- m_handler_notify_block_tip = m_node.handleNotifyBlockTip(boost::bind(BlockTipChanged, this, _1, _2, _3, _4, false));
256- m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(boost::bind(BlockTipChanged, this, _1, _2, _3, _4, true));
257+ m_handler_show_progress = m_node.handleShowProgress([this](const std::string& title, int nProgress, bool resume_possible) { ShowProgress(this, title, nProgress); });
resume_possible an unnamed parameter.
Please break this up into logical commits for review. boost::bind -> std::bind makes sense to me. Concept ACK there for sure.
std::bind -> lambda seems more like a style/preference change. I prefer lambdas too, but I don’t think that justifies the churn here. Maybe when lambdas are more robust and provide a more clear win with c++14/c++17?
Travis linter is failing with:
0Running script for: fcba334f0d21161354f168a081d95a63b82fa4c5
1ALL_FILES=$(git ls-files -- '*.cpp' '*.h')
2for i in 1 2 3 4 5; do
3 sed -i "s/ _${i}/ std::placeholders::_${i}/g" ${ALL_FILES}
4done
5fatal: ambiguous argument '5': unknown revision or path not in the working tree.
boost/bind.hpp is a very welcome first step :-)
<functional>
2376+ m_connNotifyEntryRemoved = pool.NotifyEntryRemoved.connect(std::bind(&ConnectTrace::NotifyEntryRemoved, this, std::placeholders::_1, std::placeholders::_2));
2377 }
2378
2379 ~ConnectTrace() {
2380- pool.NotifyEntryRemoved.disconnect(boost::bind(&ConnectTrace::NotifyEntryRemoved, this, _1, _2));
2381+ m_connNotifyEntryRemoved.disconnect();
boost::signals2::connection with boost::signals2::scoped_connection above.
36@@ -37,6 +37,8 @@ struct MainSignalsInstance {
37 };
38
39 static CMainSignals g_signals;
40+static std::unordered_map<CTxMemPool*, boost::signals2::connection> g_connNotifyEntryRemoved;
41+static std::unordered_map<CValidationInterface*, std::array<boost::signals2::connection, 9>> g_connMainSignals;
MainSignalsInstance members. I don’t think it is good to arbitrary divide the validationinterface state up between g_signals->m_internals and these new global variables (unless there’s a specific reason for doing this that I’m not seeing).
RegisterWithMempoolSignals() called. So I only move g_connMainSignals into a member of MainSignalsInstance. Keeping g_connNotifyEntryRemoved as a static global variable.
109- g_signals.m_internals->TransactionRemovedFromMempool.disconnect(boost::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, _1));
110- g_signals.m_internals->UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3));
111- g_signals.m_internals->NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));
112+ const auto iter = g_connMainSignals.find(pwalletIn);
113+ if (iter == g_connMainSignals.end()) return;
114+ for (const boost::signals2::connection& conn : iter->second) {
I think if you used scoped_connection instead of connection types you could get rid of this for loop.
Also, since this for loop is the only thing that appears to iterate over std::array<9> objects, it might be a good idea to replace std::array with a plain struct if you do eliminate this loop. (I noticed you used a ValidationInterfaceConnections struct in a previous version of this PR).
${i} as the script’s variable.
48
49 explicit MainSignalsInstance(CScheduler *pscheduler) : m_schedulerClient(pscheduler) {}
50 };
51
52 static CMainSignals g_signals;
53+static std::unordered_map<CTxMemPool*, boost::signals2::scoped_connection> g_connNotifyEntryRemoved;
Following up on #13743 (review), I think it would be good to add a comment saying that this has to a separate global instead of a member of MainSignalsInstance, because RegisterWithMempoolSignals is currently called before RegisterBackgroundSignalScheduler, so MainSignalsInstance hasn’t been created yet.
Init code is more messy than it needs to be and I think saying what current dependencies are will make it easier to clean up later.
utACK 52532ee43720520696d452424daad213b50aef1f. Changes since last review are adding scripted diff, removing one global, and switching to scoped signal connections.
Apparently I can’t use
${i}as the script’s variable.
It looks like this happens because commit-script-check.sh uses $i and calls eval "$SCRIPT" without a subshell. It would be nice to fix this to prevent confusion and ensure the diff checking is correct.
-BEGIN VERIFY SCRIPT-
for j in $(seq 1 5)
do
sed -i "s/ _${j}/ std::placeholders::_${j}/g" $(git grep --name-only " _${j}" -- '*.cpp' '*.h')
done
sed -i "s/boost::bind/std::bind/g" $(git grep --name-only boost::bind -- '*.cpp' '*.h')
sed -i "s/boost::ref/std::ref/g" $(git grep --name-only boost::ref -- '*.cpp' '*.h')
sed -i '/boost\/bind/d' $(git grep --name-only boost/bind)
-END VERIFY SCRIPT-