Skip to content

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Aug 11, 2014

script.h is included to define CTxDestination, which is already defined in script.h and not used here.
Look like rests from a refactor waiting to be cleaned up.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 11, 2014

This is incorrect. script.h is very clearly used. Do not assume based on a buggy comment... You must review the header for structures and defines found in script.h before proposing removal.

NAK that bit.

The typedef is a duplicate, correct.

@jtimon
Copy link
Contributor Author

jtimon commented Aug 11, 2014

What in keystore.h requires script.h?
What is class CScript; for then?

@jgarzik
Copy link
Contributor

jgarzik commented Aug 11, 2014

hmmm. As I understood it, you want to include the header for references... but looking more closely, I see all references are either typedef or virtual. There is never a need for the compiler to explore or size the structure. Apologies.

ut ACK

@jtimon
Copy link
Contributor Author

jtimon commented Aug 11, 2014

Thanks, then base58.h doesn't seem to need script.h either.

@jtimon jtimon changed the title Remove unnecessary typedef and script.h include Remove unnecessary script.h includes Aug 11, 2014
@jgarzik
Copy link
Contributor

jgarzik commented Aug 11, 2014

@jtimon script.h inclusion is only correct if the symbols are defined somewhere. The keystore.h update was correct because it included "class CScript;"

The base58.h update is incorrect, as something is required to define CScriptID and CTxDestination. This gets back to direct/indirect issue that @sipa referenced in #4674.

@jtimon
Copy link
Contributor Author

jtimon commented Aug 11, 2014

I understood the criterion in 4674, but in this case I really thought that base58.h was script.h-independent: CScriptID is declared in key.h and I missed CTxDestination. But in fact it could be easily moved with CNoDestination from script.h to key.h, since they only depends on CKeyID and CScriptID (defined in key.h).
Anyway, I'll leave the old version without touching base58 and think better how this fits with other changes I'm working on around script.

@jtimon jtimon changed the title Remove unnecessary script.h includes Remove unnecessary typedef and script.h include Aug 11, 2014
@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4680_a381ee5d1c1c01c66e681e245f60f2de2b79b264/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj laanwj merged commit a381ee5 into bitcoin:master Aug 12, 2014
laanwj added a commit that referenced this pull request Aug 12, 2014
a381ee5 Remove unnecessary typedef and script.h include (jtimon)
@jtimon jtimon deleted the keystore branch August 12, 2014 07:54
@jtimon jtimon mentioned this pull request Aug 13, 2014
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants