test: Move variable `state` down where it is used #10739

pull paveljanik wants to merge 1 commits into bitcoin:master from paveljanik:20170704_Wshadow_txvalidationcache_tests changing 1 files +1 −1

paveljanik commented at 8:16 AM on July 4, 2017: contributor

Tests added in #10192 emit few shadowing warnings:

test/txvalidationcache_tests.cpp:268:26: warning: declaration shadows a local variable [-Wshadow]
test/txvalidationcache_tests.cpp:296:26: warning: declaration shadows a local variable [-Wshadow]
test/txvalidationcache_tests.cpp:357:26: warning: declaration shadows a local variable [-Wshadow]

Remove shadowing declarations and reuse the upper local declaration as in other already present test cases.

practicalswift commented at 9:02 AM on July 4, 2017: contributor

utACK 46d53a65f8627f3d65baab00abf26bba3b6259b8
promag commented at 11:32 AM on July 4, 2017: member

Why not remove the upper declaration as it isn't used in that scope and state is not shared across those blocks?
paveljanik commented at 11:34 AM on July 4, 2017: contributor

@promag no particular reason other than loosing 3- diff status...
fanquake added the label Refactoring on Jul 4, 2017
jtimon commented at 11:04 PM on July 5, 2017: contributor

utACK 46d53a6
MarcoFalke added the label Tests on Jul 7, 2017

MarcoFalke commented at 11:22 AM on July 7, 2017: member

I'd prefer the +1-1 diff suggested by promag. This way, the state does not leak into the outer scope.

diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index a74f402..3d8a4fd 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -198,4 +198,4 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
     // spend_tx is invalid according to DERSIG
-    CValidationState state;
     {
+    CValidationState state;
         PrecomputedTransactionData ptd_spend_tx(spend_tx);

paveljanik force-pushed on Jul 8, 2017
paveljanik commented at 7:22 AM on July 8, 2017: contributor

Combined/correct fix used instead.

in src/test/txvalidationcache_tests.cpp:296 in 855adc6c94 outdated

 291 | @@ -293,7 +292,6 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 292 |  
 293 |          // Make it valid, and check again
 294 |          invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
 295 | -        CValidationState state;

promag commented at 7:24 AM on July 8, 2017:

Keep.

in src/test/txvalidationcache_tests.cpp:357 in 855adc6c94 outdated

 351 | @@ -354,7 +352,6 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 352 |          // Invalidate vin[1]
 353 |          tx.vin[1].scriptWitness.SetNull();
 354 |  
 355 | -        CValidationState state;

promag commented at 7:25 AM on July 8, 2017:

Keep.

in src/test/txvalidationcache_tests.cpp:200 in 855adc6c94 outdated

 195 | @@ -196,8 +196,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 196 |      // Test that invalidity under a set of flags doesn't preclude validity
 197 |      // under other (eg consensus) flags.
 198 |      // spend_tx is invalid according to DERSIG
 199 | -    CValidationState state;
 200 |      {
 201 | +        CValidationState state;

promag commented at 7:25 AM on July 8, 2017:

Leave the old line?

paveljanik commented at 7:36 AM on July 8, 2017:

You mean with unchanged indentation? Or?

promag commented at 9:49 AM on July 8, 2017:

Ignore, it's good.

Do not shadow upper local variable `state`. 5618b7d1ad
paveljanik force-pushed on Jul 8, 2017
promag commented at 9:50 AM on July 8, 2017: member

Obvious ACK 5618b7d.
promag commented at 9:52 AM on July 8, 2017: member

Please rename PR and commit before merge.
paveljanik renamed this:
~~Do not shadow upper local variable `state`, reuse it as in other cases.~~
Do not shadow upper local variable `state`
on Jul 8, 2017
paveljanik renamed this:
~~Do not shadow upper local variable `state`~~
Move variable `state` down where it is used
on Jul 15, 2017
sipa commented at 7:04 PM on July 15, 2017: member

utACK 5618b7d1ad3f2a258d46cf67b732ffddd3f34cb6
TheBlueMatt commented at 3:42 PM on July 16, 2017: member

utACK 5618b7d1ad3f2a258d46cf67b732ffddd3f34cb6
MarcoFalke renamed this:
~~Move variable `state` down where it is used~~
test: Move variable `state` down where it is used
on Jul 16, 2017
MarcoFalke commented at 8:47 PM on July 16, 2017: member

utACK 5618b7d
MarcoFalke merged this on Jul 16, 2017
MarcoFalke closed this on Jul 16, 2017
MarcoFalke referenced this in commit 1fc783fc08 on Jul 16, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10192 Cache full script execution results in addition to signatures