util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool

[practicalswift]

Changes in this PR:

• Add linter to make sure the return value of Parse[...](...) is checked
• Add __attribute__((warn_unused_result)) to all {Decode,Parse}[...](...) functions returning bool
• Fix violations

Context:

• wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation. #13712: wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation. would have been prevented by this

[fanquake]

$ test/lint/lint-all.sh
Missing "export LC_ALL=C" (to avoid locale dependence) as first non-comment non-empty line in test/lint/lint-cpp.sh
^---- failure generated from test/lint/lint-shell-locale.sh
The command "test/lint/lint-all.sh" exited with 1.

[practicalswift]

@fanquake Thanks! lint-shell-locale.sh to the rescue once again! Now fixed! Please re-review :-)

@Empact Good point! Updated. Please re-review :-)

[skeees]

utACK (weakly regarding the linter)
If you want to have lots of lint checks and similar static analysis to the kind you are adding here (it seems you do, and these things are useful!) - it might make sense to consider investing in building out tooling that uses libclang and inspects the AST directly as opposed to just regexp matching.

[practicalswift]

@skeees I removed the linter. Now using __attribute__((warn_unused_result)) instead. Please re-review :-)

[skeees]

:) Nice
Is is best practice to #define this attribute similar to how this is done in threadsafety.h or # define SECP256K1_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__)) ?

[practicalswift]

@skeees The secp256k1 define is used to handle ancient gcc versions AFAICS, so I see no reason to use that here. Let me know if you see any other reason :-)

[practicalswift]

Also added __attribute__((warn_unused_result)) to all Decode... functions returning bool. Please re-review :-)

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14605 (Return of the Banman by dongcarl)
• #14045 (WIP: refactor: Fix the chainparamsbase -> util circular dependency by Empact)

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.

[skeees]

can reorder the Tests* commits first before adding the attribute commits so that you don't break a bisect

[practicalswift]

@skeees When rethinking it I think you have a good point regarding using a #define. Perhaps we should go with:

#ifdef __has_cpp_attribute
#  if __has_cpp_attribute(nodiscard)
#    define NODISCARD [[nodiscard]]
#  endif
#else
#  define NODISCARD __attribute__((warn_unused_result))
#endif

That will have the benefit of being less verbose. That would use the newer [[nodiscard]] (required from C++17) where available and fall back on the older __attribute__((warn_unused_result)).

Makes sense?

What would be the most appropriate file to put that #define in?

@MarcoFalke, any input?

[practicalswift]

@skeees Commits reordered as requested!

[skeees]

I like that #define solution @practicalswift
I don't see an obviously appropriate place for these right now maybe add staticanalysis.h ?

[maflcko]

utACK 9cc0230

[Empact]

utACK 9cc0230

[sipa]

utACK 9cc0230