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
$ 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.
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:
if (gArgs.IsArgSet("-blockmintxfee") && ParseMoney(gArgs.GetArg("-blockmintxfee", ""), n)) {
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.
@skeees I removed the linter. Now using __attribute__((warn_unused_result)) instead. Please re-review :-)
:) 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__)) ?
@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 :-)
Also added __attribute__((warn_unused_result)) to all Decode... functions returning bool. Please re-review :-)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
can reorder the Tests* commits first before adding the attribute commits so that you don't break a bisect
@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?
@skeees Commits reordered as requested!
I like that #define solution @practicalswift
I don't see an obviously appropriate place for these right now maybe add staticanalysis.h ?
What about the following?
$ cat src/attributes.h
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#ifndef BITCOIN_ATTRIBUTES_H
#define BITCOIN_ATTRIBUTES_H
#ifdef __has_cpp_attribute
# if __has_cpp_attribute(nodiscard)
# define NODISCARD [[nodiscard]]
# endif
#else
# define NODISCARD __attribute__((warn_unused_result))
#endif
#endif // BITCOIN_ATTRIBUTES_H
Now using NODISCARD ([[nodiscard]] or __attribute__((warn_unused_result))) throughout. Please re-review :-)
Concept ACK, and loving 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.
@Empact Thanks for reviewing! Feedback addressed. The broken ifdef logic was sloppy from my side – sorry about that. Would you mind re-reviewing? :-)
On second thought, how about throw rather than default on unparsable option? Seems like a failure to be noisy about.
In terms of commit ordering, you need the #define NODISCARD commit to precede the commits where you use NODISCARD Other than that lgtm!
Rebased! :-) @MarcoFalke I don't know if I've thanked you before but thanks for your work on @DrahtBot! It really helps keeping the PR:s up to date! Really nice!
Updated NODISCARD to work also with Visual C++ :-)
Rebased!
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)) {
@arvidn We allow only C++11 :-)
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)
I would expect you don't need this attribute on the function definition, but just the declaration. Do you really need it here too?
ditto for all other definitions
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)
in C++ it's generally preferred to putt functions with internal linkage in the anonymous namespace instead of using static, but I see a lot of static, so perhaps that's the style here
Yes, going with static here to keep the number of lines touched lines to a minimum.
Oh, turns out 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);
These don't return bool
I see apparently unnecessary changes to: src/wallet/rpcwallet.cpp src/wallet/test/psbt_wallet_tests.cpp
@Empact Unrelated sorting of includes in those two files removed :-)
91 | @@ -92,6 +92,7 @@ endif 92 | BITCOIN_CORE_H = \ 93 | addrdb.h \ 94 | addrman.h \ 95 | + attributes.h \
Wrong file, should be 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>
Same here
attributes.h is correct but that file got lost in the last rebase. Now re-added :-)
Seems attributes.h is as same as nodiscard.h now
nodiscard.h now removed :-)
Rebased! Please re-review @ken2812221, @arvidn, @Empact, @skeees and @fanquake :-)
Updated to address @arvidn:s 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.
#if defined(__has_cpp_attribute)
# if __has_cpp_attribute(nodiscard)
# define NODISCARD [[nodiscard]]
# endif
#endif
#ifndef NODISCARD
# if defined(_MSC_VER) && _MSC_VER >= 1700
# define NODISCARD _Check_return_
# else
# define NODISCARD __attribute__((warn_unused_result))
# endif
#endif
Fixed!
Updated to address MSVC warnings. Please re-review :-)
@ken2812221 Seems like I'm unable to get rid of the ugly MSVC warnings ("unexpected tokens following preprocessor directive […]"). Do you have any theory on how to fix it? I don't have MSVC available – only AppVeyor :-)
Just put __has_cpp_attribute(...) inside the condition #ifdef __has_cpp_attribute. Like what I posted.
Rebased!
utACK 9cc0230cfc1ae9b9c1c905cd9ac613bc98bfa43a
utACK 9cc0230
utACK 9cc0230cfc1ae9b9c1c905cd9ac613bc98bfa43a