partial merge #13697, #18591, #19387, #18468: update constructors to match c++20 draft spec #4036
Conversation
This file contains hidden or 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
kwvg
changed the title
|
Neither PR title nor description mention the merge of 18388 and the partial merge of 18468. Looks good otherwise imo. |
Collaborator
Author
|
Must have forgotten, bitcoin#18388 is a partial merge as segwit specific logic wasn't merged in (source). I didn't touch bitcoin#18469 so I'm not sure about mentioning it. |
|
|
kwvg
changed the title
xdustinface
previously approved these changes
Mar 12, 2021
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Looks good overall but also gave me a bit of the partially thing feeling like mentioned in the other PR 😄
utACK
kwvg
force-pushed
the
spans
branch
2 times, most recently
from
17f15e4 to
1be361a
Compare
PastaPastaPasta
previously approved these changes
Apr 13, 2021
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK, however please squash all the related (from the same PR) commits together
Member
|
imo let's mark 18591 and 18388 as not-partial merges. The excluded changes in both are things we aren't really going to go back to later, otherwise looks good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Overview
As part of #4025, component pull requests that are required by TorV3 logic are being broken down into branches wherever feasable. This PR deals with implementation of
Span.Contents
span.h. The rest are not relevant in regards to partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 #4025 and have been skipped.Merge of span: update constructors to match c++20 draft spec and add lifetimebound attribute bitcoin/bitcoin#19387 ("span: update constructors to match c++20 draft spec and add lifetimebound attribute" by theuni)e3e7446 has been cherry-picked as 278a71c8f960337a738d45f4f39ab8663d458817 in partial merge #13815: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #4035, completing the mergeEDIT: DO NOT MERGE THIS UNTIL #4035 IS MERGED. THE PR WILL BE REBASED ON TOP OF THAT AND THEN e8782af WILL BE ADDED, THEN IT IS FIT FOR MERGEResolved with 1be361a