Re: backend *.c #include cleanup (IWYU)

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