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.

Sign up for GitHub

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

Jump to bottom

partial merge #13815: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #4035

Merged
merged 3 commits into from
Mar 22, 2021
Merged

partial merge #13815: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #4035

merged 3 commits into from
Mar 22, 2021

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Mar 10, 2021 •
edited
Loading

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 the nodiscard property.

Contents

@kwvg kwvg mentioned this pull request Mar 10, 2021
Closed
@kwvg kwvg mentioned this pull request Mar 10, 2021
Merged
UdjinM6
UdjinM6 requested changes Mar 10, 2021
View reviewed changes
src/miner.cpp Show resolved Hide resolved
@UdjinM6 UdjinM6 added this to the 17 milestone Mar 10, 2021
UdjinM6
UdjinM6 previously approved these changes Mar 10, 2021
View reviewed changes
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 suggested changes Mar 12, 2021
View reviewed changes
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.

One nit see below + I get the skipping of the test changes but whats wrong with the changes in rest.cpp?

src/bench/base58.cpp Outdated Show resolved Hide resolved
@kwvg
Copy link
Collaborator Author

kwvg commented Mar 12, 2021

@UdjinM6 I have no clue what review dismissal is or what triggers is, it seems like force pushing the branch does that

@UdjinM6
Copy link

UdjinM6 commented Mar 12, 2021

@kittywhiskers whenever you push anything to a PR that has some approvals already, these approvals are dismissed automagically. You did nothing wrong, don't worry, this is intentional (to ensure new changes were reviewed too).

@kwvg
Copy link
Collaborator Author

kwvg commented Mar 12, 2021 •
edited
Loading

Sounds a lot more authoritative then it should be. Thanks for clearing it up!

PastaPastaPasta
PastaPastaPasta requested changes Mar 17, 2021
View reviewed changes
src/rest.cpp Show resolved Hide resolved
@PastaPastaPasta
Copy link
Member

It doesn't make sense to me why e8782af is included

@kwvg
Copy link
Collaborator Author

kwvg commented Mar 20, 2021 •
edited
Loading

It doesn't make sense to me why e8782af is included

As e8782af presumes the presence of attributes.h, which isn't present in #4036 as those changes are associated with the introduction of span.h, I was forced to split bitcoin#19387 between #4035 and #4036 as bitcoin#19387 modifies both span and attribute logic, both of which are introduced in PRs where neither feature has access to the other, hence the inclusion

@PastaPastaPasta
Copy link
Member

That's okay.

Please drop e8782af from this PR, add it to 4036 PR, and then rebase 4036 on develop once we merge this PR. We should merge this pretty soon I believe. That way 19387 isn't split up into two PRs like this.

@kwvg
Copy link
Collaborator Author

kwvg commented Mar 20, 2021 •
edited
Loading

@PastaPastaPasta e8782af has been dropped, will add to #4036 once #4035 is merged in develop

PastaPastaPasta
PastaPastaPasta approved these changes Mar 20, 2021
View reviewed changes
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

UdjinM6
UdjinM6 approved these changes Mar 20, 2021
View reviewed changes
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.

re-utACK

xdustinface
xdustinface approved these changes Mar 22, 2021
View reviewed changes
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

@PastaPastaPasta PastaPastaPasta merged commit e20dc9f into dashpay:develop Mar 22, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 16, 2022
@kwvg @gades
partial merge bitcoin#13815: Add [[nodiscard]] to all {Decode,Parse}[…
0647f7b 
…...](...) functions returning bool (dashpay#4035)

* Merge bitcoin#13815: Explicitly ignore the return value of DecodeBase58(...)

bitcoin@579497e

* Merge bitcoin#13815: Default to DEFAULT_BLOCK_MIN_TX_FEE if unable to parse arg

bitcoin@7c5bc2a

* Merge bitcoin#13815: Add NODISCARD to all {Decode,Parse}[...](...) functions returning bool.

bitcoin@9cc0230
@kwvg kwvg deleted the attrib branch July 18, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@xdustinface xdustinface xdustinface approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kwvg @UdjinM6 @PastaPastaPasta @xdustinface