On failure, this makes the output more consistent with the other linters. Each failure will be marked with an ‘⚠️ ’ emoji and explanation, making it easier to spot.
Also, add –line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate different failures from each other.
Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31435.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, davidgumberg, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
349+ .expect("command error")
350+ .success();
351+ if found {
352+ Err(r#"
353+^^^
354+BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
0BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary
Concept ACK faa0b09a4b6267399a214d7f5beaf4dfe9f3976c
Good call on integrating more of the comments from the old linter into the error messages.
Tested adding assert()/CHECK_NONFATAL()/BOOST_ASSERT() with expected results in src/rpc/mining.cpp.
Might as well avoid introducing the typo (see inline comment). That’s the only light blocker for me.
nit: The whitespace changes are in the right direction, but I think they are still unclear with the current change.
0...
1All checks passed!
2src/rpc/mining.cpp:127: BOOST_ASSERT(true);
3src/rpc/mining.cpp:137: BOOST_ASSERT(true);
4
5^^^
6BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
7include of the boost/assert.hpp dependency.
8
9^---- ⚠️ Failure generated from lint check 'boost_assert'!
10Check that boost assertions are not used
11
12
13src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
14...
0...
1All checks passed!
2src/rpc/mining.cpp:127: BOOST_ASSERT(true);
3src/rpc/mining.cpp:137: BOOST_ASSERT(true);
4^^^
5BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
6include of the boost/assert.hpp dependency.
7^---- ⚠️ Failure generated from lint check 'boost_assert' (Check that boost assertions are not used)!
8
9
10src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
11...
On failure, this makes the output more consistent with the other linter.
Each failure will be marked with an '⚠️ ' emoji and explanation, making
it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate
different failures from each other.
Can be reviewed with:
"--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space"
- No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related.
- No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal).
- Keep the linter description on the same line as the failure line, otherwise it looks like it's a description for the following step.
crACK https://github.com/bitcoin/bitcoin/commit/e8f0e6efaf555a7d17bdb118464bd572bd5f3933
Tested manually with the following diff:
0diff --git a/src/coins.cpp b/src/coins.cpp
1index 24a102b0bc..000068c7e2 100644
2--- a/src/coins.cpp
3+++ b/src/coins.cpp
4@@ -21,6 +21,7 @@ std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
5
6 bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
7 {
8+ BOOST_ASSERT(1);
9 return GetCoin(outpoint).has_value();
10 }
11
12diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
13index 60828c1711..0199be420b 100644
14--- a/src/rpc/mining.cpp
15+++ b/src/rpc/mining.cpp
16@@ -133,6 +133,7 @@ static RPCHelpMan getnetworkhashps()
17
18 static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
19 {
20+ assert(1);
21 block_out.reset();
22 block.hashMerkleRoot = BlockMerkleRoot(block);
23
24diff --git a/src/rpc/server_util.cpp b/src/rpc/server_util.cpp
25index 0387cbb8e2..16b9827e13 100644
26--- a/src/rpc/server_util.cpp
27+++ b/src/rpc/server_util.cpp
28@@ -20,6 +20,7 @@ using node::NodeContext;
29
30 NodeContext& EnsureAnyNodeContext(const std::any& context)
31 {
32+ Assert(true);
33 auto node_context = util::AnyPtr<NodeContext>(context);
34 if (!node_context) {
35 throw JSONRPCError(RPC_INTERNAL_ERROR, "Node context not found");
36diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
37index f1430a3c60..079c9e0644 100644
38--- a/src/wallet/rpc/coins.cpp
39+++ b/src/wallet/rpc/coins.cpp
40@@ -467,6 +467,7 @@ RPCHelpMan getbalances()
41 const auto bal = GetBalance(wallet);
42 UniValue balances{UniValue::VOBJ};
43 {
44+ Assume(0);
45 UniValue balances_mine{UniValue::VOBJ};
46 balances_mine.pushKV("trusted", ValueFromAmount(bal.m_mine_trusted));
47 balances_mine.pushKV("untrusted_pending", ValueFromAmount(bal.m_mine_untrusted_pending));
and got the expected output:
0$ cargo run -- --lint=rpc_assert --lint=boost_assert
1src/rpc/mining.cpp:136: assert(1);
2src/rpc/server_util.cpp:23: Assert(true);
3src/wallet/rpc/coins.cpp:470: Assume(0);
4^^^
5CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
6
7Aborting the whole process is undesirable for RPC code. So nonfatal
8checks should be used over assert. See: src/util/check.h
9^---- ⚠️ Failure generated from lint check 'rpc_assert' (Check that fatal assertions are not used in RPC code)!
10
11
12src/coins.cpp:24: BOOST_ASSERT(1);
13^^^
14BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary
15include of the boost/assert.hpp dependency.
16^---- ⚠️ Failure generated from lint check 'boost_assert' (Check that boost assertions are not used)!
