test: Change background_cs from pointer to reference in validation_chainstate

MarcoFalke commented at 11:48 AM on September 29, 2021: member

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.

test: * -> &

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 https://github.com/bitcoin/bitcoin/issues/23101

Can be reviewed with --ignore-all-space.

fa4d0aacf2

MarcoFalke commented at 11:58 AM on September 29, 2021: member

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

DrahtBot added the label Tests on Sep 29, 2021

katesalazar commented at 1:41 PM on September 29, 2021: contributor

thank you!

practicalswift commented at 2:15 PM on September 29, 2021: contributor

Concept ACK

mzumsande commented at 3:53 PM on September 29, 2021: member

A description with a link to the issue it fixes would be nice.

MarcoFalke commented at 3:55 PM on September 29, 2021: member

The description is in the body of the commit message.

jnewbery commented at 4:43 PM on September 29, 2021: member

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.

MarcoFalke commented at 4:47 PM on September 29, 2021: member

Ok, copy-pasted the description

practicalswift commented at 7:55 AM on September 30, 2021: contributor

cr ACK fa4d0aacf2bbddaf1709660ffd8d520570533aa8

laanwj renamed this:
~~test: * -> &~~
test: * -> & to work around false warning in gcc 8.3
on Sep 30, 2021

laanwj commented at 10:29 AM on September 30, 2021: member

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?

MarcoFalke added the label Refactoring on Sep 30, 2021

MarcoFalke renamed this:
~~test: * -> & to work around false warning in gcc 8.3~~
test: Change background_cs from pointer to reference in validation_chainstate_tests
on Sep 30, 2021

MarcoFalke commented at 5:24 PM on September 30, 2021: member

No idea which gcc version are affected. I only tested debian-gcc-8.3

katesalazar commented at 5:39 PM on September 30, 2021: contributor

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, not by asking peers.

MarcoFalke commented at 6:24 AM on October 1, 2021: member

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.

katesalazar commented at 8:42 PM on October 11, 2021: contributor

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? @laanwj note GCC 9 at #23149 (comment).

hebasto commented at 8:54 PM on October 11, 2021: member

Concept ACK.

jamesob commented at 9:07 PM on October 11, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/23132/commits/fa4d0aacf2bbddaf1709660ffd8d520570533aa8

Reviewed code on Github.

in src/test/validation_chainstate_tests.cpp:118 in fa4d0aacf2

 120 | +                return cs;
 121 | +            }
 122 |          }
 123 | -    }
 124 | -    BOOST_CHECK(background_cs);
 125 | +        assert(false);

hebasto commented at 9:10 PM on October 11, 2021:

nit: Include <cassert> header?

MarcoFalke commented at 6:50 AM on October 12, 2021:

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.

hebasto approved

hebasto commented at 9:11 PM on October 11, 2021: member

ACK fa4d0aacf2bbddaf1709660ffd8d520570533aa8, 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 (1790a8ddacae0d52135f5020894ef1ceef625cf9):

$ 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 (1790a8ddacae0d52135f5020894ef1ceef625cf9) + 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);
      |                              ^~

MarcoFalke merged this on Oct 12, 2021

MarcoFalke closed this on Oct 12, 2021

MarcoFalke deleted the branch on Oct 12, 2021

sidhujag referenced this in commit 77e24a04a8 on Oct 12, 2021

DrahtBot locked this on Oct 30, 2022

test: Change background_cs from pointer to reference in validation_chainstate_tests #23132