refactor: cast bool operands to int to silence compiler warning #23573

jonatack commented at 1:40 PM on November 22, 2021: member

This fixes a compiler warning:

node/interfaces.cpp:544:16: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
        return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                                             &&
node/interfaces.cpp:544:16: note: cast one or both operands to int to silence this warning
node/interfaces.cpp:544:16: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
        return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                               &&
node/interfaces.cpp:544:16: note: cast one or both operands to int to silence this warning
2 warnings generated.

jonatack commented at 1:40 PM on November 22, 2021: member

A similar change was recently made to libsecp in commit 16d1322 (#1009) for the same reason.

in src/node/interfaces.cpp:546 in d6a5853bb7 outdated

 539 | @@ -540,8 +540,12 @@ class ChainImpl : public Chain
 540 |          const CBlockIndex* block2 = m_node.chainman->m_blockman.LookupBlockIndex(block_hash2);
 541 |          const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
 542 |          // Using & instead of && below to avoid short circuiting and leaving
 543 | -        // output uninitialized.
 544 | -        return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active);
 545 | +        // output uninitialized. This requires at least one of the boolean
 546 | +        // operands to be cast to int to avoid -Wbitwise-instead-of-logical
 547 | +        // compiler warnings.
 548 | +        return static_cast<int>(FillBlock(ancestor, ancestor_out, lock, active))

MarcoFalke commented at 1:48 PM on November 22, 2021:

        // output uninitialized. Cast bool to int to avoid -Wbitwise-instead-of-logical
        // compiler warnings.
        return int{FillBlock(ancestor, ancestor_out, lock, active)}

jonatack commented at 1:59 PM on November 22, 2021:

Thanks--agree, done.

jonatack commented at 2:00 PM on November 22, 2021:

One sec, mispushed.

jonatack commented at 2:12 PM on November 22, 2021:

Done.

MarcoFalke approved

MarcoFalke commented at 1:51 PM on November 22, 2021: member

ACK, though the "only one needs to be cast" seems to be an implementation detail that might change, so it would be better to not mention it nor rely on it. That is, cast all three to int. Also, I'd prefer non-narrowing casts over narrowing ones.

jonatack force-pushed on Nov 22, 2021

refactor: cast bool to int to silence compiler warning

This fixes -Wbitwise-instead-of-logical compiler warnings:

node/interfaces.cpp:544:16: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
        return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                                             &&
node/interfaces.cpp:544:16: note: cast one or both operands to int to silence this warning
node/interfaces.cpp:544:16: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
        return FillBlock(ancestor, ancestor_out, lock, active) & FillBlock(block1, block1_out, lock, active) & FillBlock(block2, block2_out, lock, active);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                               &&
node/interfaces.cpp:544:16: note: cast one or both operands to int to silence this warning
2 warnings generated.

A similar change was recently made to libsecp in commit 16d13221
for the same reason.

ab22a71429

jonatack force-pushed on Nov 22, 2021

jonatack renamed this:
~~refactor: cast bool operand to int to silence compiler warning~~
refactor: cast bool operands to int to silence compiler warning
on Nov 22, 2021

MarcoFalke approved

MarcoFalke commented at 2:17 PM on November 22, 2021: member

approach ack, didn't test nor review in detail.

theStack approved

theStack commented at 2:38 PM on November 22, 2021: member

Concept and code-review ACK ab22a71429f0f47b3c3582a303c07940aa59cd3e

The affected method ChainImpl::findCommonAncestor is only called at one place in the listsinceblock RPC, and also has a dedicated unit test interfaces_test/findCommonAncestor, i.e. after running both

$ ./src/test/test_bitcoin --run_test=interfaces_tests/findCommonAncestor
$ ./test/functional/wallet_listsinceblock.py

successfully we can be quite confident that behaviour is not changed by this commit.

DrahtBot added the label Refactoring on Nov 22, 2021

sipa commented at 4:50 PM on November 22, 2021: member

utACK ab22a71429f0f47b3c3582a303c07940aa59cd3e

shaavan approved

shaavan commented at 9:44 AM on November 23, 2021: contributor

ACK ab22a71429f0f47b3c3582a303c07940aa59cd3e

I confirmed that the changes are working correctly and as expected. I also run the following test:

$ ./src/test/test_bitcoin --run_test=interfaces_tests/findCommonAncestor
$ ./test/functional/wallet_listsinceblock.py

As suggested by @theStack and I observed no behavior change. Thanks for correcting this, @jonatack!

fanquake merged this on Nov 23, 2021

fanquake closed this on Nov 23, 2021

jonatack deleted the branch on Nov 23, 2021

sidhujag referenced this in commit 91d2638240 on Nov 23, 2021

sidhujag referenced this in commit 0bc447527c on Nov 23, 2021

deadalnix referenced this in commit fcf196f63c on Dec 23, 2021

DrahtBot locked this on Nov 23, 2022