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

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

Merged
merged 2 commits into from May 30, 2023
Merged

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

merged 2 commits into from May 30, 2023

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

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
@TheCharlatan
Copy link
Contributor

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.

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

@TheCharlatan TheCharlatan 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

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
16 checks passed
@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 @sidhujag
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCharlatan TheCharlatan TheCharlatan approved these changes

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

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maflcko @DrahtBot @TheCharlatan @stickies-v @fanquake