Skip to content

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Apr 12, 2021

Flag relevant Sock methods with [[nodiscard]] to avoid issues like the one fixed in #21631.

@vasild
Copy link
Contributor Author

vasild commented Apr 12, 2021

Having two Wait() methods - one flagged with [[nodiscard]] with 3 arguments and one without [[nodiscard]] with 2 arguments seems like too much new lines of code.

I am ~0 on this. Taking the dump/KISS approach for now - one [[nodiscard]] Wait() and cast to void the callers that don't check the return value.

cc @practicalswift

@practicalswift
Copy link
Contributor

Concept ACK

Rationale: Having the [[nodiscard]] annotation on Wait would have saved us from one uninitialized read historically. That's enough empirical evidence to convince me about the need for [[nodiscard]] here.

Thanks for addressing this!

@vasild
Copy link
Contributor Author

vasild commented Apr 13, 2021

51878b307...e286cd0d7: rebased, now #21631 is merged and I removed it from this PR

@vasild vasild marked this pull request as ready for review April 13, 2021 15:27
@practicalswift
Copy link
Contributor

cr ACK e286cd0: the only changes made are additions of [[nodiscard]] and (void) where appropriate

@DrahtBot DrahtBot mentioned this pull request May 7, 2021
12 tasks
@DrahtBot
Copy link
Contributor

DrahtBot commented May 7, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@laanwj
Copy link
Member

laanwj commented May 19, 2021

Code review ACK e286cd0

@laanwj laanwj merged commit 39d597d into bitcoin:master May 19, 2021
@vasild vasild deleted the Sock_nodiscard branch May 19, 2021 16:02
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 19, 2021
e286cd0 net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)

Pull request description:

  Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in bitcoin#21631.

ACKs for top commit:
  practicalswift:
    cr ACK e286cd0: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
  laanwj:
    Code review ACK e286cd0

Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants