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;
Please remove :-)
29 | @@ -28,6 +30,8 @@ 30 | #include <QDebug> 31 | #include <QTimer> 32 | 33 | +using namespace std::placeholders;
Please remove :-)
25 | @@ -24,6 +26,8 @@ 26 | #include <QPainter> 27 | #include <QRadialGradient> 28 | 29 | +using namespace std::placeholders;
Please remove :-)
28 | @@ -27,6 +29,8 @@ 29 | #include <QIcon> 30 | #include <QList> 31 | 32 | +using namespace std::placeholders;
Please remove :-)
28 | @@ -27,6 +29,7 @@ 29 | #include <QSet> 30 | #include <QTimer> 31 | 32 | +using namespace std::placeholders;
Please remove :-)
27 | @@ -27,6 +28,8 @@ 28 | #include <event2/event.h> 29 | #include <event2/thread.h> 30 | 31 | +using namespace std::placeholders;
Please remove :-)
I believe it is acceptable to be using using namespace std::placeholders here, since the alternative would be extremely verbose.
@MarcoFalke I'm not sure I agree. The verbosity could help remind us that we really should use lambdas where we're currently using std::bind (modulo the few places where that is not technically possible in C++11), no? :-)
@practicalswift Since @MarcoFalke think that using namespace std::placeholders; is acceptable, I will wait for more comments from other people and decide what to do.
@ken2812221 OK! :-) Could you try rewriting them as lambdas where appropriate? That would get rid of std::bind too :-)
@practicalswift Good point! I'll try to use lambda to replace bind
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;
nit: can remove, this is now unreferenced
How about 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);
nit: Could 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();
nit: could extract these as a method on 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));
nit: could extract as a method on ValidationInterfaceConnections
Concept ACK.
5 | @@ -6,10 +6,13 @@ 6 | #ifndef BITCOIN_VALIDATIONINTERFACE_H 7 | #define BITCOIN_VALIDATIONINTERFACE_H 8 | 9 | +#include <boost/signals2/signal.hpp> 10 | +
I believe this will slow down the build significantly. So tend to NACK here.
Fixed
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Could you sum up the differences between 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); });
Make 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:
Running script for: fcba334f0d21161354f168a081d95a63b82fa4c5
ALL_FILES=$(git ls-files -- '*.cpp' '*.h')
for i in 1 2 3 4 5; do
sed -i "s/ _${i}/ std::placeholders::_${i}/g" ${ALL_FILES}
done
fatal: ambiguous argument '5': unknown revision or path not in the working tree.
Update: In this PR, I do "replace boost::bind to std::bind" only because there are some disputes about lambda.
@ken2812221 Makes sense! Getting rid of boost/bind.hpp is a very welcome first step :-)
Concept ACK, the callers should include <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();
Could drop this disconnect line (and the whole destructor) by replacing 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;
It seems like these two maps would most naturally be 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).
I was trying to do this however MainSignalsInstance is not created when 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).
That idea looks good to me. I'll take it.
utACK d4a4f5729f0faa3bc51c6f162a10812c9703d5a5. I also think it'd be good to switch to lambdas in the future.
Can this be implemented as a scripted diff? It looks relatively straightforward.
@sipa Done. Apparently I can't use ${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.
DoneDone
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-utACK cb53b825c26af6e628ba88d72b2000e75bedbbc6. Only change is last review is new comment.
utACK cb53b825c26af6e628ba88d72b2000e75bedbbc6
utACK cb53b82
utACK cb53b825c26af6e628ba88d72b2000e75bedbbc6