Changes in this PR:
Parse[...](...) is checked__attribute__((warn_unused_result)) to all {Decode,Parse}[...](...) functions returning boolContext:
wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation. would have been prevented by this
0$ test/lint/lint-all.sh
1Missing "export LC_ALL=C" (to avoid locale dependence) as first non-comment non-empty line in test/lint/lint-cpp.sh
2^---- failure generated from test/lint/lint-shell-locale.sh
3The command "test/lint/lint-all.sh" exited with 1.
76- options.blockMinFeeRate = CFeeRate(n);
77+ if (ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
78+ options.blockMinFeeRate = CFeeRate(n);
79+ } else {
80+ options.blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
81+ }
You could avoid the duplicate else clause by boosting amount out like so:
0if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
__attribute__((warn_unused_result)) instead. Please re-review :-)
#define this attribute similar to how this is done in threadsafety.h or # define SECP256K1_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__)) ?
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 :-)
__attribute__((warn_unused_result)) to all Decode… functions returning bool. Please re-review :-)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
@skeees When rethinking it I think you have a good point regarding using a #define. Perhaps we should go with:
0#ifdef __has_cpp_attribute
1# if __has_cpp_attribute(nodiscard)
2# define NODISCARD [[nodiscard]]
3# endif
4#else
5# define NODISCARD __attribute__((warn_unused_result))
6#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?
#define solution @practicalswift
I don’t see an obviously appropriate place for these right now maybe add staticanalysis.h ?
What about the following?
0$ cat src/attributes.h
1// Copyright (c) 2009-2010 Satoshi Nakamoto
2// Copyright (c) 2009-2018 The Bitcoin Core developers
3// Distributed under the MIT software license, see the accompanying
4// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5
6#ifndef BITCOIN_ATTRIBUTES_H
7#define BITCOIN_ATTRIBUTES_H
8
9#ifdef __has_cpp_attribute
10# if __has_cpp_attribute(nodiscard)
11# define NODISCARD [[nodiscard]]
12# endif
13#else
14# define NODISCARD __attribute__((warn_unused_result))
15#endif
16
17#endif // BITCOIN_ATTRIBUTES_H
NODISCARD ([[nodiscard]] or __attribute__((warn_unused_result))) throughout. Please re-review :-)
NODISCARD. But you need to add attributes.h to src/Makefile.am. Also please sort your new include alphabetically.
5+
6+#ifndef BITCOIN_ATTRIBUTES_H
7+#define BITCOIN_ATTRIBUTES_H
8+
9+#ifdef __has_cpp_attribute
10+# if __has_cpp_attribute(nodiscard)
You should collapse this to one level: #if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)
Otherwise NODISCARD will be undefined if __has_cpp_attribute(nodiscard) is false.
ifdef logic was sloppy from my side – sorry about that. Would you mind re-reviewing? :-)
#define NODISCARD commit to precede the commits where you use NODISCARD Other than that lgtm!
NODISCARD to work also with Visual C++ :-)
69@@ -70,9 +70,8 @@ static BlockAssembler::Options DefaultOptions()
70 // If -blockmaxweight is not given, limit to DEFAULT_BLOCK_MAX_WEIGHT
71 BlockAssembler::Options options;
72 options.nBlockMaxWeight = gArgs.GetArg("-blockmaxweight", DEFAULT_BLOCK_MAX_WEIGHT);
73- if (gArgs.IsArgSet("-blockmintxfee")) {
74- CAmount n = 0;
75- ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n);
76+ CAmount n = 0;
77+ if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
with this change the scope of variable n expands and “leaks” outside of where it’s used.
perhaps not a big deal, but if we allow C++17 this could be written as:
if (CAmount n = 0; gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
105@@ -105,7 +106,7 @@ static std::string AvailableDataFormatsString()
106 return formats;
107 }
108
109-static bool ParseHashStr(const std::string& strReq, uint256& v)
110+NODISCARD static bool ParseHashStr(const std::string& strReq, uint256& v)
430@@ -431,7 +431,7 @@ std::vector<Span<const char>> Split(const Span<const char>& sp, char sep)
431 }
432
433 /** Parse a key path, being passed a split list of elements (the first element is ignored). */
434-bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out)
435+NODISCARD static bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath& out)
static, but I see a lot of static, so perhaps that’s the style here
static here to keep the number of lines touched lines to a minimum.
ParseKeyPath(…) is already in the anonymous namespace. Thus making static redundant. Now removed. Thanks for the nit @arvidn!
36@@ -35,8 +37,8 @@ enum SafeChars
37 * @return A new string without unsafe chars
38 */
39 std::string SanitizeString(const std::string& str, int rule = SAFE_CHARS_DEFAULT);
40-std::vector<unsigned char> ParseHex(const char* psz);
41-std::vector<unsigned char> ParseHex(const std::string& str);
42+NODISCARD std::vector<unsigned char> ParseHex(const char* psz);
43+NODISCARD std::vector<unsigned char> ParseHex(const std::string& str);
bool
91@@ -92,6 +92,7 @@ endif
92 BITCOIN_CORE_H = \
93 addrdb.h \
94 addrman.h \
95+ attributes.h \
nodiscard.h?
attributes.h is correct but that file got lost in the last rebase. Now re-added :-)
13@@ -14,6 +14,8 @@
14 #ifndef BITCOIN_BASE58_H
15 #define BITCOIN_BASE58_H
16
17+#include <attributes.h>
attributes.h is correct but that file got lost in the last rebase. Now re-added :-)
attributes.h is as same as nodiscard.h now
nodiscard.h now removed :-)
static nit. Please re-review :-)
11+#elif defined(_MSC_VER) && _MSC_VER >= 1700
12+# define NODISCARD _Check_return_
13+#else
14+# define NODISCARD __attribute__((warn_unused_result))
15+#endif
16+
This solution might be too complex, but it can get rid of annoying MSVC warnings.
0#if defined(__has_cpp_attribute)
1# if __has_cpp_attribute(nodiscard)
2# define NODISCARD [[nodiscard]]
3# endif
4#endif
5#ifndef NODISCARD
6# if defined(_MSC_VER) && _MSC_VER >= 1700
7# define NODISCARD _Check_return_
8# else
9# define NODISCARD __attribute__((warn_unused_result))
10# endif
11#endif
