Skip to content

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Aug 11, 2014

Cleaning up includes that don't seem necessary anymore.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 11, 2014

Several of the local includes are quite obviously used in script, such as crypto. NAK as-is.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4674_54f048158b1b9864b5119c5f7dc3f126182288c0/ 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.

@jtimon
Copy link
Contributor Author

jtimon commented Aug 11, 2014

Those are included in hash, which is included in key, which is included in keystore.
Otherwise the build would fail, no?

@sipa
Copy link
Member

sipa commented Aug 11, 2014

You always want to include all direct dependencies of a file. Doing so
makes it 1) clear which code you're depending on and 2) makes the codebase
robust against refactoring (for example, A depends on B, A and B both
depend on C; B is changed to no longer need C... if A didn't include C
directly, this would cause build failures).

As a general rule, an indirect dependency should not hide a direct
dependency. There is one exception: if A.cpp and A.h both depend on B, it
is omitted in the .cpp file (the .cpp and .h file are considered one unit).

@jgarzik
Copy link
Contributor

jgarzik commented Aug 11, 2014

Using an #include says "I am using this API" It does not matter if other headers also use that API; you cannot depend on that fact, as indirect dependencies may change.

This change makes the build more fragile and misunderstands how standard C/C++ header inclusion works.

@jgarzik jgarzik closed this Aug 11, 2014
@jtimon jtimon deleted the includescript branch August 11, 2014 12:06
@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