partial merge #13815: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by kwvg · Pull Request #4035 · dashpay/dash

kwvg · 2021-03-10T05:17:53Z

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.

Partial merge util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool bitcoin/bitcoin#13815 ("util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool" by practicalswift)
- Cannot be fully merged yet as 145fe95 presumes the presence of bitcoin@4f8704d, not critical in the context of partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 #4025 so 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)
- A majority of span: update constructors to match c++20 draft spec and add lifetimebound attribute bitcoin/bitcoin#19387 has been committed in partial merge #13697, #18591, #19387, #18468: update constructors to match c++20 draft spec #4036, completing the PR

src/miner.cpp

UdjinM6

utACK

xdustinface

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

…58(...) bitcoin@579497e

… parse arg bitcoin@7c5bc2a

kwvg · 2021-03-12T17:47:53Z

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

UdjinM6 · 2021-03-12T18:07:34Z

@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 · 2021-03-12T18:11:24Z

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

src/rest.cpp

…nctions returning bool. bitcoin@9cc0230

PastaPastaPasta · 2021-03-19T21:38:12Z

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

kwvg · 2021-03-20T05:00:34Z

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 · 2021-03-20T05:16:05Z

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 · 2021-03-20T16:11:50Z

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

PastaPastaPasta

utACK

UdjinM6

re-utACK

xdustinface

utACK

…...](...) 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 mentioned this pull request Mar 10, 2021

partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 #4025

Closed

kwvg force-pushed the attrib branch from 04dff9a to 278a71c Compare March 10, 2021 05:46

kwvg mentioned this pull request Mar 10, 2021

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

Merged

UdjinM6 requested changes Mar 10, 2021

View reviewed changes

src/miner.cpp Outdated Show resolved Hide resolved

kwvg force-pushed the attrib branch from 278a71c to f2d62e7 Compare March 10, 2021 17:24

UdjinM6 added this to the 17 milestone Mar 10, 2021

UdjinM6 previously approved these changes Mar 10, 2021

View reviewed changes

UdjinM6 requested review from PastaPastaPasta and xdustinface March 10, 2021 23:48

xdustinface requested changes Mar 12, 2021

View reviewed changes

src/bench/base58.cpp Outdated Show resolved Hide resolved

kwvg added 2 commits March 12, 2021 23:16

Merge bitcoin#13815: Explicitly ignore the return value of DecodeBase…

2380070

…58(...) bitcoin@579497e

Merge bitcoin#13815: Default to DEFAULT_BLOCK_MIN_TX_FEE if unable to…

c0845e5

… parse arg bitcoin@7c5bc2a

kwvg dismissed UdjinM6’s stale review via dc62d4d March 12, 2021 17:46

kwvg force-pushed the attrib branch from f2d62e7 to dc62d4d Compare March 12, 2021 17:46

PastaPastaPasta requested changes Mar 17, 2021

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

Merge bitcoin#13815: Add NODISCARD to all {Decode,Parse}[...](...) fu…

eb35113

…nctions returning bool. bitcoin@9cc0230

kwvg force-pushed the attrib branch from dc62d4d to e8782af Compare March 19, 2021 05:30

kwvg force-pushed the attrib branch from e8782af to eb35113 Compare March 20, 2021 16:11

kwvg requested a review from PastaPastaPasta March 20, 2021 16:12

PastaPastaPasta approved these changes Mar 20, 2021

View reviewed changes

UdjinM6 approved these changes Mar 20, 2021

View reviewed changes

xdustinface approved these changes Mar 22, 2021

View reviewed changes

PastaPastaPasta merged commit e20dc9f into dashpay:develop Mar 22, 2021

kwvg deleted the attrib branch July 18, 2023 11:38

Conversation

kwvg commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Contents

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwvg commented Mar 12, 2021

Uh oh!

UdjinM6 commented Mar 12, 2021

Uh oh!

kwvg commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta commented Mar 19, 2021

Uh oh!

kwvg commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PastaPastaPasta commented Mar 20, 2021

Uh oh!

kwvg commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwvg commented Mar 10, 2021 •

edited

Loading

kwvg commented Mar 12, 2021 •

edited

Loading

kwvg commented Mar 20, 2021 •

edited

Loading

kwvg commented Mar 20, 2021 •

edited

Loading