test: Change background_cs from pointer to reference in validation_chainstate_tests #23132
Conversation
This changes background_cs from being a pointer to a reference to work around a gcc false warning. Also, this makes the test easier to read. Fixes bitcoin#23101 Can be reviewed with --ignore-all-space.
|
This is fixed in gcc8.4, I believe, but debian:buster still ships 8.3, in case anyone wants to test this: https://packages.debian.org/buster/gcc |
|
thank you! |
|
Concept ACK |
|
A description with a link to the issue it fixes would be nice. |
|
The description is in the body of the commit message. |
I agree that the description/link to issue should appear in the PR description, to make it easier for other contributors to quickly understand what the PR is doing. |
|
Ok, copy-pasted the description |
|
cr ACK fa4d0aa |
|
Concept ACK I've tried to make the PR title clearer, but I don't know: does this only affect gcc 8.3 or a wider version range? |
|
No idea which gcc version are affected. I only tested debian-gcc-8.3 |
|
Nobody needs to sweat it, who cares, simply keep GCC 8.3 for now. Ideally, you would find out the answer by reading CI workers' logs, |
|
We don't have the resources to maintain CI scripts for every operating system and every compiler version, so I think asking which version is affected seems reasonable. |
@laanwj note GCC 9 at #23149 (comment). |
|
Concept ACK. |
|
ACK fa4d0aa Reviewed code on Github. |
There was a problem hiding this comment.
ACK fa4d0aa, tested on Linux Mint 20.2 (x86_64) by merging this PR on top of the current master.
Compiler:
$ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- master (1790a8d):
$ make > /dev/null
test/fuzz/float.cpp: In function ‘void float_fuzz_target(FuzzBufferType)’:
test/fuzz/float.cpp:60:30: warning: ‘tmp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
60 | assert(std::isnan(d) || d == d_deserialized);
| ^~
In file included from ./validation.h:15,
from ./test/util/chainstate.h:13,
from test/validation_chainstate_tests.cpp:11:
./chain.h: In member function ‘void validation_chainstate_tests::chainstate_update_tip::test_method()’:
./chain.h:422:27: warning: ‘background_cs’ may be used uninitialized in this function [-Wmaybe-uninitialized]
422 | return vChain.size() > 0 ? vChain[vChain.size() - 1] : nullptr;
| ~~~~~~~~~~~^~
test/validation_chainstate_tests.cpp:110:18: note: ‘background_cs’ was declared here
110 | CChainState* background_cs;
| ^~~~~~~~~~~~~
- master (1790a8d) + this PR:
$ make > /dev/null
test/fuzz/float.cpp: In function ‘void float_fuzz_target(FuzzBufferType)’:
test/fuzz/float.cpp:60:30: warning: ‘tmp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
60 | assert(std::isnan(d) || d == d_deserialized);
| ^~
| } | ||
| } | ||
| BOOST_CHECK(background_cs); | ||
| assert(false); |
There was a problem hiding this comment.
It would be nicer to be able to run iwyu on the codebase rather than spend time on reviewing includes. Also, assert is already used in this file, so this seems unrelated.
This changes background_cs from being a pointer to a reference to work
around a gcc false warning. Also, this makes the test easier to read.
Fixes #23101
Can be reviewed with --ignore-all-space.