doc: Refine header include policy #12933

Since there is no harm in having "duplicate" includes and it makes it obvious what are the dependencies of each file, without having to do static analysis or jumping between files, I'd suggest to revert the travis check for duplicate includes.

Generally, I think that enforcing minor style preferences should not be done via travis. The cost of maintaining and the burden on other developers is too high. C.f discussion in #10973 (review)

Attn @practicalswift since this is removing the script added in #11878.

utACK fa783dc74e7a8d8dd9ed8be1b621db7af7cebd33, because I'm fine with this change in its current form, but I do think it would be a little better to keep the lint-includes.sh script and only drop the DUPLICATE_INCLUDES_IN_HEADER_AND_CPP_FILES section of the script. I think the checks the script does within source files are useful and not a burden. It's only the checks between corresponding .h and .cpp files that break compatibility with iwyu tool and seem like not good practice.

Also @sipa since this is changing the guideline added in #10575.

I agree. I think we went overboard with this.

I think the checks the script does within source files are useful and not a burden

I don't think having a duplicate include will ever happen. Even if it does happen, there is no harm and it can be easily fixed once every couple of years.

@MarcoFalke The PR title feels a bit misleading since the main part of this PR is the policy change (the policy introduced by @sipa in #10575). The linting is just a "regression test" for that policy.

What about changing the title to "Change #include policy" or similar?

NACK

I think the header include policy introduced by @sipa in #10575 is reasonable.

Assuming that we keep @sipa:s policy then having a regression test for it in Travis to guard against newly introduced violations seems reasonable as well.

Having a Travis check for developer note violations will avoid having human reviewers point out nits/violations that could have be addressed pre-human-review.

Remember: 1.) reviewer time is more scarce than developer time, 2.) computing time is cheap :-)

What about changing the title to "Change #include policy" or similar?

Thanks, done.

@practicalswift I am only aware of three cases of actual duplicate includes in the last three years ( #10840, #10561 and #6187) and I don't think it is worth to keep the lint check, since there is no harm in having those.

Ok, whatever. Kept the duplicate include check.

@MarcoFalke If the suggested policy is that we should have Travis checks only for things that cause harm in a strict technical sense, then it follows that we should remove lint-whitespace.sh and some of the other linters too?

I honestly don't understand the desire to limit automated testing. Is this because we expect the linters to cause false positives and/or to break so often that we need to actively maintain them? If a linter causes false positives, it should simply be removed, period, and I have a hard time seeing a simple shell script breaking very often.

Personally I think we should automate anything that can be automated, in order to maintain a clean workspace for everyone. If someone who makes a PR is turned off by having to address automated whine, which is exclusively very simple to do, then chances are they'll be turned off by nits or review anyway.

@MarcoFalke If the suggested policy is that we should have Travis checks only for things that cause harm in a strict technical sense, then it follows that we should remove lint-whitespace.sh and some of the other linters too?

Checking for trailing whitespace is done, since some code editors remove that whenever you open/close a file. That burdens the developers that have this setting turned on in their editors, as we want patches to be free of unrelated changes whenever possible. (They'd have to go in and undo the whitespace change or turn off the setting ...)

@kallewoof The current changes in this pull keep the duplicate include check linter. I am no longer removing it.

utACK fad0fc3c9a9759dfb2bb1bdf1aaa5e1d08c0ab9c

Going to merge this to fix annoying travis failures for some develpers, since my merge of #11878 was too quick apparently.

Further refinement of the policy or linter scripts can be done in follow up pull requests.

Going to merge this

Premature in my opinion. This changes the style policy and had one ACK and one NACK. Better to wait until consensus is clear.

@jnewbery I see 3 ACKS (laanwj, ryanofsky, and myself) and one NACK (practicalswift). kallewoof seemed to have some general disagreement about removing linters and I assumed it was related to a previous version of my pull request, which removed the whole linter script.

Since my merge of #11878 caused issues for any dev using iwyu, I wanted to get in the travis fix rather sooner than later. With regard to the policy: As said above, I am happy to have a follow-up pull request to further refine the policy.

I see 3 ACKS (laanwj, ryanofsky, and myself)

wumpus's ACK was prior to your last commit changing the policy, and a PR author's ACK doesn't usually count :)

Note that this also partially reverts #10575, which had many more ACKs.

I'm not saying merging this is the wrong move, just that it's premature without wider consensus.

@jnewbery Do you think it could be possible to find a solution that satisfies the NACKs at the same time?

@MarcoFalke Yes, I should have clarified. I was talking about the general aversion for automated linters and tests to make the code more uniform, not this particular PR.

I also think in general a PR should not be merged by its author if it is contentious in any way, to ensure that people are in agreement.

There are 3 different questions here:

1. What the header include guideline should be.
2. Whether that guideline is to be enforced as a policy for all PRs.
3. Whether that policy should be automatically enforced by a linter.

My impression is that this PR seems to have been created out of a displeasure with (3), and overreacting by changing (1) and (2) as well.

As for the guideline itself, I think there was no need for change. The don't-reinclude-in-cpp-what-h-already-includes approach has been in use since #2767 and was made explicit in #10575 with many ACKs. I think that is a very reasonable approach: the .h and .cpp file together are one module, with the .cpp containing the non-public part. The only reason I see for changing that policy is if we want to encourage occasional use of iwyu, which AFAIK does not support that policy. However, for now it does not seem very straightforward to use.

I want to apologize if this change was not in agreement or controversial. I primarily wanted a quick fix after I realized that #11878 was breaking existing workflows and resulted in confusing travis failures.

Is there anything I could do to address peoples dislike of my merge?

@sipa Thanks for your input.

Would everyone be happy if my changes to the developer guidelines are reverted, but the additional travis linter check left out (for now at least)?

Would everyone be happy if my changes to the developer guidelines are reverted, but the additional travis linter check left out (for now at least)?

I'd be unhappy about this (but obviously could live with it) because I like to use the iwyu tool which is incompatible with it, and also because I think previous developer guideline was very strange. I've been writing c++ a long time, and "include what you use" has always just seemed like the obvious thing to do when you want the the benefits described here: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

The "include what you use mostly, but not if the same include line is present in a different file" policy seems seems complicated and unusual to me, and I don't see what benefits it offers. I also don't see any rationales that were given in favor of it over plain iwyu in #10575 or #2767, though admittedly I've only just skimmed those threads.

@ryanofsky The wording seems to be lax about it and doesn't forbid iwyu or requires linters to be run.

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. One exception is that a .cpp file does not need to re-include the includes already included in its corresponding .h file.

(emphasis mine)

Note that it doesn't use "must" and "must not".

Please provide further feedback and review in #12960

I primarily wanted a quick fix...

That's understood. I certainly didn't mean to question your reasons for merging - I just wanted to point out that in my opinion it wasn't necessary to merge quite so quickly since there was an outstanding concern from one of the regular contributors.

At the risk of complicating this further - should the guidelines attempt to specify if/when a forward declare is preferable over an include - there are a bunch of existing forward declares and these notes seem to suggest that an include should always be preferable

@skeees I think you are free to use forward declares whenever they are sufficient and it helps to reduce the number of headers that are included in that header (and thus in the places where the header is used). Though, I think it is not worth to enforce this in any way.