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.

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

validation: Document where we are intentionally ignoring bool return values from validation related functions #13864

Conversation

Projects
None yet
4 participants
@practicalswift
Copy link
Member

commented Aug 3, 2018

  • Document where we are intentionally ignoring the return value from bool returning functions from validation.{cpp,h} that modify state
  • Mark validation functions whose bool return value should not be discarded in the general case with NODISCARD (expands to [[nodiscard]] or __attribute__((warn_unused_result)) depending on availability)

This will help us identify code where we're we incorrectly assume that a certain function always succeeds in performing its state modification.

@Empact

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Should we not update the callers to react and log warnings on failures?

E.g. this seems like a meaningful case:

if (!file) {
    LogPrintf("Warning: Could not open blocks file %s\n", path.string());
} else {
    LogPrintf("Importing blocks file %s...\n", path.string());
    if (!LoadExternalBlockFile(chainparams, file)) {
        LogPrintf("Warning: No blocks loaded from %s\n", path.string());
    }
}
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14624 (Some simple improvements to the RNG code by sipa)
  • #14605 (Return of the Banman by dongcarl)
  • #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)
  • #14194 (Annotate unused parameters with [[maybe_unused]] by practicalswift)
  • #14193 (validation: Add missing mempool locks by MarcoFalke)
  • #13937 (Track best-possible-headers (TheBlueMatt) by Sjors)
  • #13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
  • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
  • #13804 (WIP: Transaction Pool Layer by MarcoFalke)

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.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2018

@Empact Increased logging should perhaps be a subject for another PR. Personally I’m not convinced that these cases are worth logging for – nobody seems to have been missing these log messages so far and we should be careful to decrease the signal to noise in our logging. It is already very verbose :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2018

Rebased! Please review :-)

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch Aug 25, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 26, 2018

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch Aug 27, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

Updated NODISCARD to work also with Visual C++ :-)

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch Sep 21, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

Rebased! :-)

@DrahtBot DrahtBot removed the Needs rebase label Sep 21, 2018

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch Sep 21, 2018

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch Oct 24, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

Rebased! :-)

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Oct 24, 2018

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

Rebased! :-)

@practicalswift practicalswift force-pushed the practicalswift:nodiscard-for-bool-returning-funcs-mutating-arguments branch to 71e5ce5 Oct 28, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.