Skip to content

Conversation

@practicalswift
Copy link
Contributor

  • 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
Copy link
Contributor

Empact 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
Copy link
Contributor

DrahtBot 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
Copy link
Contributor Author

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

Rebased! Please review :-)

@practicalswift practicalswift force-pushed the nodiscard-for-bool-returning-funcs-mutating-arguments branch from 72fbc1a to e551e4d Compare August 25, 2018 23:11
@practicalswift practicalswift force-pushed the nodiscard-for-bool-returning-funcs-mutating-arguments branch from e551e4d to 339e8b6 Compare August 27, 2018 17:13
@practicalswift
Copy link
Contributor Author

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

@practicalswift
Copy link
Contributor Author

Rebased! :-)

@practicalswift
Copy link
Contributor Author

Rebased! :-)

@practicalswift
Copy link
Contributor Author

Rebased! :-)

@practicalswift practicalswift force-pushed the nodiscard-for-bool-returning-funcs-mutating-arguments branch from 81a168c to 71e5ce5 Compare October 28, 2018 16:33
@practicalswift practicalswift deleted the nodiscard-for-bool-returning-funcs-mutating-arguments branch April 10, 2021 19:36
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 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.

4 participants