Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

partial merge #13697, #18591, #19387, #18468: update constructors to match c++20 draft spec #4036

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Mar 10, 2021

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

EDIT: 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 MERGE Resolved with 1be361a

@kwvg kwvg changed the title Spans partial merge #13697, #18591, #19387: update constructors to match c++20 draft spec Mar 10, 2021
@UdjinM6
Copy link

UdjinM6 commented Mar 10, 2021

Neither PR title nor description mention the merge of 18388 and the partial merge of 18468. Looks good otherwise imo.

@kwvg
Copy link
Collaborator Author

kwvg commented Mar 10, 2021

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.

@UdjinM6
Copy link

UdjinM6 commented Mar 10, 2021

18469 18468 🙈

@kwvg kwvg changed the title partial merge #13697, #18591, #19387: update constructors to match c++20 draft spec partial merge #13697, #18591, #19387, #18468: update constructors to match c++20 draft spec Mar 10, 2021
@UdjinM6 UdjinM6 added this to the 17 milestone Mar 10, 2021
UdjinM6
UdjinM6 previously approved these changes Mar 10, 2021
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

xdustinface
xdustinface previously approved these changes Mar 12, 2021
Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall but also gave me a bit of the partially thing feeling like mentioned in the other PR 😄

utACK

@kwvg kwvg dismissed stale reviews from xdustinface and UdjinM6 via be1929d March 23, 2021 04:22
@kwvg kwvg force-pushed the spans branch 2 times, most recently from 17f15e4 to 1be361a Compare March 23, 2021 04:24
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit, LGTM otherwise

configure.ac Outdated Show resolved Hide resolved
UdjinM6
UdjinM6 previously approved these changes Mar 23, 2021
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

utACK (compiles with and without --enable-c++17 locally with no issues)

EDIT: remote build (on ubuntu) with --enable-c++17 fails though, #4055 should fix it

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, however please squash all the related (from the same PR) commits together

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Apr 22, 2021

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

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants