refactor: Replace boost::bind with std::bind #13743

ken2812221 · 2018-07-22T15:19:19Z

Replace boost::bind with std::bind

In src/rpc/server.cpp, replace std::transform with simple loop.
In src/validation.cpp, store the boost::signals2::connection object and use it to disconnect.
In src/validationinterface.cpp, use 2 map to store the boost::signals2::scoped_connection object.

fanquake · 2018-07-23T07:31:33Z

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?

practicalswift · 2018-07-23T07:53:52Z

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 as std::string.

ken2812221 · 2018-07-23T10:35:02Z

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

src/qt/bitcoingui.cpp

src/qt/clientmodel.cpp

src/qt/splashscreen.cpp

src/qt/transactiontablemodel.cpp

src/qt/walletmodel.cpp

src/torcontrol.cpp

maflcko · 2018-07-23T11:34:54Z

I believe it is acceptable to be using using namespace std::placeholders here, since the alternative would be extremely verbose.

practicalswift · 2018-07-23T11:56:05Z

@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? :-)

ken2812221 · 2018-07-23T12:17:10Z

@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.

practicalswift · 2018-07-23T13:26:34Z

@ken2812221 OK! :-) Could you try rewriting them as lambdas where appropriate? That would get rid of std::bind too :-)

ken2812221 · 2018-07-23T13:49:22Z

@practicalswift Good point! I'll try to use lambda to replace bind

Empact · 2018-07-23T17:25:34Z

src/rpc/server.cpp

@@ -508,9 +507,10 @@ std::vector<std::string> CRPCTable::listCommands() const
    std::vector<std::string> commandList;
    typedef std::map<std::string, const CRPCCommand*> commandMap;


nit: can remove, this is now unreferenced

Empact · 2018-07-23T17:28:46Z

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.

Empact · 2018-07-23T17:31:17Z

src/validationinterface.cpp

+    connections.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2));
+    connections.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
+    connections.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));
+    g_signals.m_wallet_connections.emplace(pwalletIn, connections);


nit: Could std::move connections

Empact · 2018-07-23T17:40:30Z

src/validationinterface.cpp

+    connections.ChainStateFlushed.disconnect();
+    connections.Broadcast.disconnect();
+    connections.BlockChecked.disconnect();
+    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.

Empact · 2018-07-23T17:42:32Z

src/validationinterface.cpp

+    connections.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, _1));
+    connections.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2));
+    connections.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
+    connections.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2));


nit: could extract as a method on ValidationInterfaceConnections

promag · 2018-07-24T00:14:07Z

Concept ACK.

maflcko · 2018-07-24T00:21:41Z

src/validationinterface.h

@@ -6,10 +6,13 @@
 #ifndef BITCOIN_VALIDATIONINTERFACE_H
 #define BITCOIN_VALIDATIONINTERFACE_H

+#include <boost/signals2/signal.hpp>
+


I believe this will slow down the build significantly. So tend to NACK here.

DrahtBot · 2018-07-25T15:01:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14464 (refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) by ken2812221)
#14384 (Resolve validationinterface circular dependencies by 251Labs)
#14035 (Utxoscriptindex by mgrychow)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)

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.

ryanofsky

utACK cb53b82. Only change is last review is new comment.

sipa · 2018-10-23T02:10:11Z

utACK cb53b82

ryanofsky · 2018-12-04T16:41:23Z

Can this be merged, or does it need more review? I see review from @theuni was requested in github, but I don't know if it's necessary given the other reviews since this change is pretty straightforward.

fanquake · 2018-12-29T04:59:16Z

utACK cb53b82

practicalswift · 2018-12-29T10:30:43Z

utACK cb53b82

cb53b82 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee) 2196c51 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee) Pull request description: Replace boost::bind with std::bind - In `src/rpc/server.cpp`, replace `std::transform` with simple loop. - In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect. - In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object. Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0

boost::bind usage was removed in bitcoin#13743. However a new usage snuck in as part of 2bc4c3e (bitcoin#15225).

e36f802 lint: add C++ code linter (fanquake) c4be50f remove usage of boost::bind (fanquake) Pull request description: `boost::bind` usage was removed in #13743. However a new usage snuck in as part of 2bc4c3e (#15225). ACKs for top commit: hebasto: ACK e36f802 practicalswift: ACK e36f802 -- patch looks correct Tree-SHA512: 2b0387c5443c184bcbf7df4849db1ed1296ff82c7b4ff0aff18334a400e56a472a972d18234d3866531a088d7a8da64688e58dc9f15daaad4048697c759d55ce

-BEGIN VERIFY SCRIPT- for j in $(seq 1 5) do gsed -i "s/ _${j}/ std::placeholders::_${j}/g" $(git grep --name-only " _${j}" -- '*.cpp' '*.h') done gsed -i "s/boost::bind/std::bind/g" $(git grep --name-only boost::bind -- '*.cpp' '*.h') gsed -i "s/boost::ref/std::ref/g" $(git grep --name-only boost::ref -- '*.cpp' '*.h') gsed -i '/boost\/bind/d' $(git grep --name-only boost/bind) -END VERIFY SCRIPT-

… in signal/slot, also prefer range-based loop instead of std::transform # Conflicts: # src/validationinterface.cpp

…l/slot, also prefer range-based loop instead of std::transform

merge bitcoin#15118, bitcoin#14172, bitcoin#15623, bitcoin#14121, partial bitcoin#13743, partial bitcoin#15280: block filters

cb53b82 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee) 2196c51 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee) Pull request description: Replace boost::bind with std::bind - In `src/rpc/server.cpp`, replace `std::transform` with simple loop. - In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect. - In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object. Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0

…nd (#4343) * Merge bitcoin#13743: refactor: Replace boost::bind with std::bind cb53b82 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee) 2196c51 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee) Pull request description: Replace boost::bind with std::bind - In `src/rpc/server.cpp`, replace `std::transform` with simple loop. - In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect. - In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object. Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0 * Replace boost::bind with std::bind and remove Boost.Bind includes Co-authored-by: MarcoFalke <falke.marco@gmail.com>

laanwj added the Refactoring label Jul 22, 2018

fanquake changed the title ~~[Refactor] Replace some boost::bind to std::bind~~ refactor: Replace some boost::bind to std::bind Jul 23, 2018