Hi, On 2024-02-10 08:40:43 +0100, Peter Eisentraut wrote: > So as a test, I ran IWYU over the backend *.c files and removed all the > #includes it suggested. (Note that IWYU also suggests to *add* a bunch of > #includes, in fact that is its main purpose; I didn't do this here.) In some > cases, a more specific #include replaces another less specific one. (To > keep the patch from exploding in size, I ignored for now all the suggestions > to replace catalog/pg_somecatalog.h with catalog/pg_somecatalog_d.h.) This > ended up with the attached patch, which has
I think this kind of suggested change is why I have been wary of IWYU (and the changes that clangd suggest): By moving indirect includes to .c files you end up with implementation details more widely, which can increase the pain of refactoring. I'd be hesitant to commit to this without a) a policy about adding annotations about when indirect includes shouldn't directly be included b) annotations ensuring that. What were the parameters you used for IWYU? I'm e.g. a bit surprised about the changes to main.c, adding an include for s_lock.h. For one I don't think s_lock.h should ever be included directly, but more importantly it seems there isn't a reason to include spin.h either, but it's not removed here? There are other changes I don't understand. E.g. why is catalog/binary_upgrade.h removed from pg_enum.c? It's actually defining binary_upgrade_next_pg_enum_oid, declared in catalog/binary_upgrade.h? I think some of the changes here could be applied independently of more iwyu infrastructure, e.g. replacing includes of builtins.h with includes of fmgrprotos.h. But the larger set of changes seems somewhat hard to judge as-is. FWIW, for me the tree with the patch applied doesn't build anymore, presumably due to 8be93177c46. ../../../../../home/andres/src/postgresql/src/backend/commands/event_trigger.c: In function 'EventTriggerOnLogin': ../../../../../home/andres/src/postgresql/src/backend/commands/event_trigger.c:955:72: error: 'F_OIDEQ' undeclared (first use in this function) 955 | BTEqualStrategyNumber, F_OIDEQ, | ^~~~~~~ ../../../../../home/andres/src/postgresql/src/backend/commands/event_trigger.c:955:72: note: each undeclared identifier is reported only once for each function it appears in Greetings, Andres Freund