Just some minor test-only refactor commits to fix GCC false positive warnings, along with making the test code easier to read and understand:
Those changes are required in a bunch of pulls touching the CI system, so merging them allows to drop them in all pulls.
It does not make sense to use a pointer, when a reference is more
appropriate, especially given that nullptr has been ruled out.
This is also allows to remove the CI workaround to avoid warnings:
```
C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
...
/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_method()’:
/ci_container_base/src/test/blockmanager_tests.cpp:63:17: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~~~
In file included from /ci_container_base/src/streams.h:13,
from /ci_container_base/src/dbwrapper.h:11,
from /ci_container_base/src/node/blockstorage.h:10,
from /ci_container_base/src/test/blockmanager_tests.cpp:8:
/ci_container_base/src/util/check.h:116:49: note: the temporary was destroyed at the end of the full expression ‘inline_assertion_check<true, std::unique_ptr<ChainstateManager>&>(((blockmanager_tests::blockmanager_scan_unlink_already_pruned_files*)this)->blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::<anonymous>.TestChain100Setup::<anonymous>.TestingSetup::<anonymous>.ChainTestingSetup::<anonymous>.BasicTestingSetup::m_node.node::NodeContext::chainman, std::source_location{(& *.Lsrc_loc27)}, std::basic_string_view<char>(((const char*)"m_node.chainman")))’
116 | #define Assert(val) inline_assertion_check<true>(val, std::source_location::current(), #val)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/src/test/blockmanager_tests.cpp:63:28: note: in expansion of macro ‘Assert’
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:382: src/test/CMakeFiles/test_bitcoin.dir/blockmanager_tests.cpp.obj] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1810: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake: *** [Makefile:146: all] Error 2
```
This false-positive warning is also fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M1300 | @@ -1301,7 +1301,7 @@ class V2TransportTester 1301 | { 1302 | // Construct contents consisting of 0x00 + 12-byte message type + payload. 1303 | std::vector<uint8_t> contents(1 + CMessageHeader::MESSAGE_TYPE_SIZE + payload.size()); 1304 | - std::copy(mtype.begin(), mtype.end(), reinterpret_cast<char*>(contents.data() + 1)); 1305 | + std::copy(mtype.begin(), mtype.end(), contents.begin() + 1); 1306 | std::copy(payload.begin(), payload.end(), contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
To clarify that the second one doesn't actually copy where the first left off:
std::ranges::copy(mtype, contents.begin() + 1);
std::ranges::copy(payload, contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
The goal is to just change the line, so that it matches all other test cases in this file. Happy to review a follow-up to apply the clang-tidy use-ranges to the src/test directory (or all files). But I want to keep this one minimal for now.
1300 | @@ -1301,7 +1301,7 @@ class V2TransportTester
1301 | {
1302 | // Construct contents consisting of 0x00 + 12-byte message type + payload.
We could mention that the fixed-size type is right-padded with zeros:
// [0x00][12-byte message type, zero padded on right][payload]
I think that is clear from reading the code, so will leave as-is for now. No opinion about string_view. Will leave as-is for now, to not invalidate the two reviews.
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3
I left some nits for the second commit, but I'm also fine merging as is:
mtype param can be std::string_viewmtype.size() should be documented to be less or equal to the available spacestd::ranges::copy would simplify the copy and clarify that we jump more than the number of inserted bytes before0 (the 1 + part of the offsets) we could redundantly add populate the first byte as well.void SendMessage(std::string_view mtype, std::span<const uint8_t> payload)
{
BOOST_REQUIRE_LE(mtype.size(), CMessageHeader::MESSAGE_TYPE_SIZE);
// [0x00][12-byte message type, zero padded on right][payload]
std::vector<uint8_t> contents(1 + CMessageHeader::MESSAGE_TYPE_SIZE + payload.size());
contents[0] = 0;
std::ranges::copy(mtype, contents.begin() + 1);
std::ranges::copy(payload, contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
SendPacket(contents);
}
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33840.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--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.
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3, I have reviewed the code and it looks OK.