Skip to content

Conversation

paveljanik
Copy link
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
Copy link
Contributor

utACK 46d53a65f8627f3d65baab00abf26bba3b6259b8

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@paveljanik
Copy link
Contributor Author

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

@jtimon
Copy link
Contributor

jtimon commented Jul 5, 2017

utACK 46d53a6

@maflcko maflcko added the Tests label Jul 7, 2017
@maflcko
Copy link
Member

maflcko 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 paveljanik force-pushed the 20170704_Wshadow_txvalidationcache_tests branch from 46d53a6 to 855adc6 Compare July 8, 2017 07:21
@paveljanik
Copy link
Contributor Author

Combined/correct fix used instead.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep.

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

CValidationState state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep.

{
CValidationState state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave the old line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean with unchanged indentation? Or?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore, it's good.

@paveljanik paveljanik force-pushed the 20170704_Wshadow_txvalidationcache_tests branch from 855adc6 to 5618b7d Compare July 8, 2017 07:30
@promag
Copy link
Contributor

promag commented Jul 8, 2017

Obvious ACK 5618b7d.

@promag
Copy link
Contributor

promag commented Jul 8, 2017

Please rename PR and commit before merge.

@paveljanik paveljanik changed the title Do not shadow upper local variable state, reuse it as in other cases. Do not shadow upper local variable state Jul 8, 2017
@paveljanik paveljanik changed the title Do not shadow upper local variable state Move variable state down where it is used Jul 15, 2017
@sipa
Copy link
Member

sipa commented Jul 15, 2017

utACK 5618b7d

1 similar comment
@TheBlueMatt
Copy link
Contributor

utACK 5618b7d

@maflcko maflcko changed the title Move variable state down where it is used test: Move variable state down where it is used Jul 16, 2017
@maflcko
Copy link
Member

maflcko commented Jul 16, 2017

utACK 5618b7d

@maflcko maflcko merged commit 5618b7d into bitcoin:master Jul 16, 2017
maflcko pushed a commit that referenced this pull request Jul 16, 2017
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants