Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 12, 2018

It was noted in #12933 that the change to the header include policy was controversial and not agreed upon.

This reverts the guideline to its initial state.

@maflcko maflcko added the Docs label Apr 12, 2018
@maflcko
Copy link
Member Author

maflcko commented Apr 12, 2018

Can be reviewed by checking out the commit and running:

$ git diff HEAD~2 HEAD ./doc/ | wc
      0       0       0

@practicalswift
Copy link
Contributor

ACK fa548e7

@sipa
Copy link
Member

sipa commented Apr 12, 2018

@ryanofsky (responding to your comment in #12933) As I mentioned, the ability to use iwyu seems like a good argument in favor of changing the policy. But then we should:

  • Actually discuss changing the policy, independently from the issue with the linter annoyance
  • Provide instructions/scripts/... on how to use iwyu

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 12, 2018

To get specific, I guess the immediate question is whether to keep the sentence:

One exception is that a .cpp file does not need to re-include the includes already included in its corresponding .h file.

As Marco pointed out, since it says "does not need to" instead of "shouldn't", this isn't actually incompatible with iwyu like I originally thought in the other issue, so I wouldn't object to it anymore.

If there is going to be a bigger rewrite, I would like to see something like:

Every .cpp and .h file should directly #include header files it needs definitions from, without relying on transitive includes from unrelated headers. See WhyIWYU for various considerations and rationale, and consider running the IWYU tool to automatically remove unneeded includes and add/remove forward declarations.

Admittedly, it is a little awkward right now to use iwyu with bitcoin, though I regularly do, and I previously posted instructions at #11878 (comment). It might be possible to add some build system support that would make iwyu easier to run, but I'd have to look into that.

@laanwj
Copy link
Member

laanwj commented Apr 12, 2018

Can we please not spend so much time on this :/

The back and forth on this, merging and reverting, why is this (which is pretty much just a style issue) suddenly such a contended topic.

@maflcko maflcko closed this Apr 13, 2018
@maflcko maflcko deleted the Mf1804-docIncludes branch April 13, 2018 13:56
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants