On 2023/01/20 13:41, Jakub Zelenka <bu...@php.net> wrote: > I think you should clarify in the RFC how this is going to be split and > what's the time frame for getting that all in.
The RFC is not the actual cleanup, it's for reaching an agreement on whether it is a desirable goal to have minimal includes instead of the chaotic mess we currently have. This was negated by Dmitry Stogov and Derick Rethans (but by nobody else so far; the discussion here was mostly about other aspects, but not about this basic questions). After this disagreement among PHP maintainers, Christoph M. Becker suggested doing an RFC: https://github.com/php/php-src/pull/10220#issuecomment-1383789100 The splitting and time frame are out of the RFC's scope. I'll do whatever reviewers ask me to do, that's why I splitted my first PRs (after being asked to do so). > Your commits are separated by files and are really too small. I didn't mean > that. What I was talking about it is to do that single change and apply all > dependent changes on it so it still compiles / works and can be proposed as > a separate chunk of work. It means if you remove "zend_gc.h" from "zend.h", > then your PR would also contain changes that add "zend_gc.h" to "zend_gc.c" > and "zend_objects_API.h". So to get "zend.h" to this state in you PR, it > would require 9 PR's if I count correctly. There would be some other > changes after that obviously but it should be nowhere near to 104... I did it the other way round, an approach which makes much more sense IMHO: (For example) I created a stgit patch which cleans up zend.h to only include headers that are needed by zend.h itself. That broke the build of various other libraries, for example "zend_gc.c". For me, this was clearly a sign that "zend_gc.c" is buggy; adding the missing "#include" here had actually nothing to do with removing one from "zend.h". So I popped the "zend.h" patch from the stack, created a new patch to fix up "zend_gc.c". Back to the "zend.h" patch, build again, and create more patches to fix libraries that got broken. That way, each patch fixes the bugs in one library, a bug that was invisible before because of the header bloat, but gets later revealed by header fixups. Each per-library patch is simple to review: it adds #includes to the ".c" file which were missing there; the diff shows which are added, and a reviewer can verify that those headers are indeed needed there. The same patch may remove unneeded #includes from the ".h" file, which the reviewer can also easily verify. Each per-library patch is buildable. You can go back and forth, "git checkout" each of my commits and you have a working PHP source tree, with commits that are self-contained, are reviewable and make sense. Your approach is harder to review, because there is nothing a reviewer can do to verify the correctness, other than building PHP. They cannot see whether the list of added #includes is really complete, because they see only those that got added, but cannot see those that are still missing. Max -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
