Skip to content

refactor: Add [[nodiscard]] where ignoring a Result return type is an error#27774

Merged
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2305-nodiscard-result-error-
May 30, 2023
Merged

refactor: Add [[nodiscard]] where ignoring a Result return type is an error#27774
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2305-nodiscard-result-error-

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 29, 2023

Only add it for those where it is an error to ignore. Also, fix the gcc compile warning #25977 (comment). Also, fix iwyu for touched header files.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 29, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27711 (kernel: Remove shutdown from kernel library by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot changed the title refactor: Add [[nodiscard]] where ignoring a Result return type is an error refactor: Add [[nodiscard]] where ignoring a Result return type is an error May 29, 2023
@sedited
Copy link
Contributor

sedited commented May 29, 2023

Only add it for those where it is an error to ignore.

How did you select these?

@maflcko
Copy link
Member Author

maflcko commented May 29, 2023

How did you select these?

I asked my brain if it thought that ignoring the return type would lead to errors in later code.

sedited
sedited approved these changes May 29, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK fa5680b

@DrahtBot DrahtBot mentioned this pull request May 29, 2023
Closed
stickies-v
stickies-v approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK fa5680b

I looked at all sites where Result is either of type void or some other descriptive/status type, and could not find any other places where the return value can never be ignored.

stickies-v
stickies-v approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(hit send twice - duplicate post)

@fanquake fanquake merged commit 214f8f1 into bitcoin:master May 30, 2023
@maflcko maflcko deleted the 2305-nodiscard-result-error- branch May 30, 2023 14:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 30, 2023
@fanquake
Merge bitcoin#27774: refactor: Add [[nodiscard]] where ignoring a Res…
4b69edb 
…ult return type is an error

fa5680b fix includes for touched header files (iwyu) (MarcoFalke)
dddde27 Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke)

Pull request description:

  Only add it for those where it is an error to ignore. Also, fix the gcc compile warning bitcoin#25977 (comment). Also, fix iwyu for touched header files.

ACKs for top commit:
  TheCharlatan:
    ACK fa5680b
  stickies-v:
    ACK fa5680b

Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 15, 2023
@ryanofsky @PiRK
refactor: Replace std::optional<bilingual_str> with util::Result
f3139dd 
Summary:
> refactor: Replace std::optional<bilingual_str> with util::Result

bitcoin/bitcoin@8aa8f73

> Add [[nodiscard]] where ignoring a Result return type is an error

bitcoin/bitcoin@dddde27

> fix includes for touched header files (iwyu)

bitcoin/bitcoin@fa5680b

This is a partial backport of [[bitcoin/bitcoin#25977 | core#25977]] and [[bitcoin/bitcoin#27774 | core#27774]]

Test Plan:
`ninja all check-all`

`grep -r 'std::optional<bilingual_str>'  src/`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D14992
@bitcoin bitcoin locked and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

+1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @sedited @stickies-v @fanquake