refactor: conver std::vector const references to Span #5336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
356988e util: make EncodeBase58Check consume Spans (Sebastian Falbesoner) f0fce06 util: make EncodeBase58 consume Spans (Sebastian Falbesoner) Pull request description: This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs bitcoin#19660, bitcoin#19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks). ACKs for top commit: laanwj: Code review ACK 356988e Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
fa4827a to
d751b27
Compare
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
May 10, 2024
, bitcoin#22829, bitcoin#24079, bitcoin#24108, bitcoin#24157, bitcoin#25109 (network backports: part 5) 5dde8e7 merge bitcoin#25109: Strengthen AssertLockNotHeld assertions (Kittywhiskers Van Gogh) a1f005e merge bitcoin#24157: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it (Kittywhiskers Van Gogh) de4b4bf merge bitcoin#24108: Replace RecursiveMutex cs_addrLocal with Mutex, and rename it (Kittywhiskers Van Gogh) 2f7a138 merge bitcoin#24079: replace RecursiveMutex cs_SubVer with Mutex (and rename) (Kittywhiskers Van Gogh) 23b152c merge bitcoin#22829: various RecursiveMutex replacements in CConnman (Kittywhiskers Van Gogh) 362e310 merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes (Kittywhiskers Van Gogh) bf98ad6 merge bitcoin#22782: Remove unused MaybeSetAddrName (Kittywhiskers Van Gogh) 2b65526 merge bitcoin#21167: make CNode::m_inbound_onion public, initialize explicitly (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6001 * Dependency for #6018 * Partially reverts ff69e0d from #5336 due to `Span<CNode*>`'s incompatibility with `CConnman::NodesSnapshot::Snap()` (returning `const std::vector<CNode*>&`) ``` masternode/sync.cpp:147:18: error: no matching member function for call to 'RequestGovernanceObjectVotes' m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman); ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./governance/governance.h:360:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'CNode &' for 1st argument int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const; ^ ./governance/governance.h:361:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'Span<CNode *>' for 1st argument int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const; ^ 1 error generated. ``` * Dash already implements its own `CNode*` iteration logic in [dash#1382](#1382) and implemented additional capabilities in [dash#1575](#1575), which meant backporting [bitcoin#21943](bitcoin#21943) involved migrating Dash-specific code to upstream logic that needed to be modified to implement expected functionality. * Unlike Bitcoin, Dash maintains a map of every raw `SOCKET` corresponding to a pointer of their `CNode` instance and uses it to translate socket sets to their corresponding `CNode*` sets. This is done to accommodate for edge-triggered modes which have an event-socket relationship, as opposed to level-triggered modes, which have a socket-event relationship. This means that `CConnman::SocketHandlerConnected()` doesn't require access to a vector of all `CNode` pointers and therefore, the argument `nodes` has been omitted. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5dde8e7 Tree-SHA512: 5685d8ebb4fa1f10d018e60d9b0efc3100ea13ac437e7892a09ad3f86d6ac6756e4b5a08ebe70de2eabb27740678e10b975d319f2d553ae5b27dafa71dba0a9f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
refactoring to convert std::vector const ref parameters into Spans
What was done?
How Has This Been Tested?
make -j8 check
Breaking Changes
Should be none
Checklist: