I am in both camps. I think some slight cleanup might be in order, but not how you are proposing it. First of all: - I am against forward struct declarations. I think they decrease code readability and should be avoided. - I am against putting comments on #includes. Comments are noise in code and often go out of date. Especially in a situation like this where the comment is physically far away from the code that introduced it. To take the example: > #include "zend_portability.h" // for BEGIN_EXTERN_C What if in future the need for BEGIN_EXTERN_C disappears? Who is going to remember to update the comment?
As you said yourself, this refactoring has no practical effect on compilation times. But your RFC states this as one of the advantages. This is misleading. Another advantage listed is "more correct code". Perhaps it is, but does it mean that the current situation is likely to cause more frequent PHP bugs? Would this refactoring help in finding PHP bugs more quicker? Others have raised a valid point that PHP bugs are fixed in earlier versions and then merged up into master. What you are proposing is going to lead to issues with merging. And I don't mean merge conflicts, because includes rarely change in bug fixes. I mean things like a bug fix using a symbol that hasn't been used in the file but is included through one of the more generic headers. Once such a commit is merged into master, it may turn out that the symbol lacks a new include. This adds unnecessary work for bug fixers who will now have to verify this and find the appropriate include during merges. So maybe reduce scope of this refactoring? Do it only where it's strictly necessary. Don't use forward declarations or comments. Or maybe have all ZEND headers included with a single header? Refactoring needs to make developers' life easier and not just be done for the sake of following best-practice.
