Header include guideline #10575

sipa · 2017-06-11T22:53:53Z

No description provided.

gmaxwell

ACK

practicalswift · 2017-06-12T02:25:53Z

ACK!

fanquake · 2017-06-12T04:39:52Z

ACK a090d1c

paveljanik · 2017-06-12T05:02:30Z

ACK a090d1c

jonasschnelli · 2017-06-12T07:16:39Z

Good!
ACK a090d1c

laanwj · 2017-06-12T07:42:35Z

Makes sense.

Is there a good way to check/review this, though? Would be useful to add suggestion for reviewing.

I'm also afraid this will generate lots of 'mess around with headers' PRs, so we probably want to add that this holds for new code.

practicalswift · 2017-06-12T09:01:28Z

@laanwj I wrote the following script ...

#!/bin/bash

for H in $(git grep "" -- "*.h" | cut -f1 -d: | uniq); do
    CPP=${H/%\.h/.cpp}
    if [[ ! -e $CPP ]]; then
        continue
    fi
    cat "$H" "$CPP" | grep -E "^#include " | sort | uniq -c | \
      grep " 2 #include" && echo "$CPP" && echo
done

... to identify .cpp files that re-include the includes already included in its corresponding .h file. See PR #10574.

laanwj · 2017-06-12T09:53:33Z

Well yes but I'm more interested in the first point:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other
definitions from, even if those headers are already included indirectly through other headers.

So how to check that a .cpp or .h is using functionality that is only indirectly included? Seems like a lot of work to do manually, at least.

practicalswift · 2017-06-12T10:24:00Z

@laanwj Ah, iwyu is probably best-in-class there!

ryanofsky · 2017-06-12T13:03:27Z

Iwyu is great and it would be really nice if it works on bitcoin source. It manages forward declarations as well as includes and will automatically add/remove/convert between them as appropriate.

sipa · 2017-06-12T18:47:53Z

Perhaps we should have iwyu included in our build system, and then refer to it in developer-notes.md?

laanwj · 2017-06-13T07:19:33Z

Iwyu is great and it would be really nice if it works on bitcoin source

Has anyone tried?

Edit: anyhow, that discussion does not have to block this. If you get that tool to work, it would be appreciated if you file a PR how to use it with our source.

a090d1c Header include guideline (Pieter Wuille) Tree-SHA512: 44c46a3e249c946303b0fa45ddeba1abc40ec4f993b78f10894d6f43de2b62c493d74f8a24b5b69d3c71cd5c1b3cdb638c8eabdade3dc60e376bc933a8f10940

…ing .h file already included a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift) Pull request description: Remove includes in .cpp files for things the corresponding .h file already included. Example case: * `addrdb.cpp` includes `addrdb.h` and `fs.h` * `addrdb.h` includes `fs.h` Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`. In line with the header include guideline (see #10575). Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346

@sipa

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in #10575). Example run: ``` $ git diff diff --git a/src/warnings.cpp b/src/warnings.cpp index c52a1fd..d8994dd 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -5,6 +5,8 @@ #include <sync.h> #include <clientversion.h> +#include <string> #include <util.h> #include <warnings.h> +#include <util.h> diff --git a/src/warnings.h b/src/warnings.h index e8e982c..8d2252e 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -7,6 +7,7 @@ #define BITCOIN_WARNINGS_H #include <stdlib.h> #include <string> +#include <stdlib.h> void SetMiscWarning(const std::string& strWarning); $ contrib/devtools/lint-includes.sh Duplicate include(s) in src/warnings.h: #include <stdlib.h> Include(s) from src/warnings.h duplicated in src/warnings.cpp: #include <string> Duplicate include(s) in src/warnings.cpp: #include <util.h> $ echo $? 1 $ git checkout . $ contrib/devtools/lint-includes.sh $ echo $? 0 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

@sipa

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

@sipa

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

a090d1c Header include guideline (Pieter Wuille) Tree-SHA512: 44c46a3e249c946303b0fa45ddeba1abc40ec4f993b78f10894d6f43de2b62c493d74f8a24b5b69d3c71cd5c1b3cdb638c8eabdade3dc60e376bc933a8f10940

…responding .h file already included a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift) Pull request description: Remove includes in .cpp files for things the corresponding .h file already included. Example case: * `addrdb.cpp` includes `addrdb.h` and `fs.h` * `addrdb.h` includes `fs.h` Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`. In line with the header include guideline (see bitcoin#10575). Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575). Example run: ``` $ git diff diff --git a/src/warnings.cpp b/src/warnings.cpp index c52a1fd..d8994dd 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -5,6 +5,8 @@ #include <sync.h> #include <clientversion.h> +#include <string> #include <util.h> #include <warnings.h> +#include <util.h> diff --git a/src/warnings.h b/src/warnings.h index e8e982c..8d2252e 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -7,6 +7,7 @@ #define BITCOIN_WARNINGS_H #include <stdlib.h> #include <string> +#include <stdlib.h> void SetMiscWarning(const std::string& strWarning); $ contrib/devtools/lint-includes.sh Duplicate include(s) in src/warnings.h: #include <stdlib.h> Include(s) from src/warnings.h duplicated in src/warnings.cpp: #include <string> Duplicate include(s) in src/warnings.cpp: #include <util.h> $ echo $? 1 $ git checkout . $ contrib/devtools/lint-includes.sh $ echo $? 0 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

@sipa

c36b720 Add Travis check for duplicate includes (practicalswift) 280023f Remove duplicate includes (practicalswift) Pull request description: This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575). Example run: ``` $ git diff diff --git a/src/warnings.cpp b/src/warnings.cpp index c52a1fd..d8994dd 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -5,6 +5,8 @@ #include <sync.h> #include <clientversion.h> +#include <string> #include <util.h> #include <warnings.h> +#include <util.h> diff --git a/src/warnings.h b/src/warnings.h index e8e982c..8d2252e 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -7,6 +7,7 @@ #define BITCOIN_WARNINGS_H #include <stdlib.h> #include <string> +#include <stdlib.h> void SetMiscWarning(const std::string& strWarning); $ contrib/devtools/lint-includes.sh Duplicate include(s) in src/warnings.h: #include <stdlib.h> Include(s) from src/warnings.h duplicated in src/warnings.cpp: #include <string> Duplicate include(s) in src/warnings.cpp: #include <util.h> $ echo $? 1 $ git checkout . $ contrib/devtools/lint-includes.sh $ echo $? 0 ``` Tree-SHA512: f653d23c58ebc024dfc5b1fb8570698fd3c515c75b60b5cabbc43595548c488fca92349fa4c8b64460edbe61c879ff1d24f37f959e18552e202a7342460ddbf1

fanquake added the Docs label Jun 11, 2017

Header include guideline

a090d1c

sipa force-pushed the includeguide branch from e2d8143 to a090d1c Compare June 11, 2017 22:55

gmaxwell approved these changes Jun 11, 2017

View reviewed changes

practicalswift mentioned this pull request Jun 12, 2017

Remove includes in .cpp files for things the corresponding .h file already included #10574

Merged

laanwj merged commit a090d1c into bitcoin:master Jun 13, 2017

sipa deleted the includeguide branch June 23, 2017 00:34

practicalswift mentioned this pull request Dec 12, 2017

Add Travis check for duplicate includes #11878

Merged

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Apr 9, 2018

Add Travis check for duplicate includes

c36b720

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

ryanofsky mentioned this pull request Apr 10, 2018

doc: Refine header include policy #12933

Merged

stamhe pushed a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018

Add Travis check for duplicate includes

76e9046

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 15, 2018

Add Travis check for duplicate includes

0128379

This enforces parts of the project header include guidelines (added by @sipa in bitcoin#10575).

HashUnlimited mentioned this pull request Aug 15, 2018

Add Travis check for duplicate includes chaincoin/chaincoin#172

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Header include guideline #10575

Header include guideline #10575

Uh oh!

sipa commented Jun 11, 2017

Uh oh!

gmaxwell left a comment

Uh oh!

practicalswift commented Jun 12, 2017

Uh oh!

fanquake commented Jun 12, 2017

Uh oh!

paveljanik commented Jun 12, 2017

Uh oh!

jonasschnelli commented Jun 12, 2017

Uh oh!

laanwj commented Jun 12, 2017

Uh oh!

practicalswift commented Jun 12, 2017 •

edited

Loading

Uh oh!

laanwj commented Jun 12, 2017

Uh oh!

practicalswift commented Jun 12, 2017 •

edited

Loading

Uh oh!

ryanofsky commented Jun 12, 2017

Uh oh!

sipa commented Jun 12, 2017

Uh oh!

laanwj commented Jun 13, 2017 •

edited

Loading

Uh oh!

Uh oh!

Header include guideline #10575

Header include guideline #10575

Uh oh!

Conversation

sipa commented Jun 11, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 12, 2017

Uh oh!

fanquake commented Jun 12, 2017

Uh oh!

paveljanik commented Jun 12, 2017

Uh oh!

jonasschnelli commented Jun 12, 2017

Uh oh!

laanwj commented Jun 12, 2017

Uh oh!

practicalswift commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 12, 2017

Uh oh!

practicalswift commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jun 12, 2017

Uh oh!

sipa commented Jun 12, 2017

Uh oh!

laanwj commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jun 12, 2017 •

edited

Loading

practicalswift commented Jun 12, 2017 •

edited

Loading

laanwj commented Jun 13, 2017 •

edited

Loading