/bitcoin

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

Merged
merged 1 commit into from Jul 16, 2017
+1 −1

Conversation

Reviewers

@promag promag

Assignees
No one assigned
Labels
Refactoring Tests
Projects
None yet
Milestone
No milestone
8 participants
@paveljanik @practicalswift @jtimon @MarcoFalke @promag @sipa @TheBlueMatt @fanquake
@paveljanik
Contributor

paveljanik commented Jul 4, 2017

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
Contributor

practicalswift commented Jul 4, 2017

utACK 46d53a6
@promag

promag reviewed Jul 4, 2017
View changes

Why not remove the upper declaration as it isn't used in that scope and state is not shared across those blocks?

@paveljanik
Contributor

paveljanik commented Jul 4, 2017

@promag no particular reason other than loosing 3- diff status...

fanquake added the Refactoring label Jul 4, 2017

@jtimon
Member

jtimon commented Jul 5, 2017

utACK 46d53a6

MarcoFalke added the Tests label Jul 7, 2017

@MarcoFalke
Member

MarcoFalke commented Jul 7, 2017

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
Contributor

paveljanik commented Jul 8, 2017

Combined/correct fix used instead.
src/test/txvalidationcache_tests.cpp
{
+ CValidationState state;
@promag

promag Jul 8, 2017

Contributor

Leave the old line?

@paveljanik

paveljanik Jul 8, 2017

Contributor

You mean with unchanged indentation? Or?

@promag

promag Jul 8, 2017

Contributor

Ignore, it's good.

src/test/txvalidationcache_tests.cpp
@@ -293,7 +292,6 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
// Make it valid, and check again
invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
- CValidationState state;
@promag

promag Jul 8, 2017

Contributor

Keep.

src/test/txvalidationcache_tests.cpp
@@ -354,7 +352,6 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
// Invalidate vin[1]
tx.vin[1].scriptWitness.SetNull();
- CValidationState state;
@promag

promag Jul 8, 2017

Contributor

Keep.

@promag
Contributor

promag commented Jul 8, 2017 • edited

Obvious ACK 5618b7d.
@promag
Contributor

promag commented Jul 8, 2017

Please rename PR and commit before merge.

paveljanik changed the title from Do not shadow upper local variable `state`, reuse it as in other cases. to Do not shadow upper local variable `state` Jul 8, 2017

paveljanik changed the title from Do not shadow upper local variable `state` to Move variable `state` down where it is used Jul 15, 2017

@sipa
Owner

sipa commented Jul 15, 2017

utACK 5618b7d
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 16, 2017

utACK 5618b7d

MarcoFalke changed the title from Move variable `state` down where it is used to test: Move variable `state` down where it is used Jul 16, 2017

@MarcoFalke
Member

MarcoFalke commented Jul 16, 2017

utACK 5618b7d

@MarcoFalke MarcoFalke merged commit 5618b7d into bitcoin:master Jul 16, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke added a commit that referenced this pull request Jul 16, 2017

@MarcoFalke MarcoFalke Merge #10739: test: Move variable `state` down where it is used 
5618b7d Do not shadow upper local variable `state`. (Pavel Janík)

Pull request description:

  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.

Tree-SHA512: 1e3c52cf963f8f33e729900c8ecdcd5cc6fe28caa441ba53c4636df9cc3d1a351ca231966d36384589f1340ae8ddd447424c2ee3e8527d334d0412f0d1a10c8f
This commit was signed with a verified signature.
@MarcoFalke MarcoFalke
GPG key ID: D2EA4850E7528B25 Learn about signing commits
1fc783f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment