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

On 12.02.24 21:07, Andres Freund wrote:
> 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'm actually not clear on what the policy of catalog/pg_somecatalog.h 
versus catalog/pg_somecatalog_d.h is or should be.  There doesn't appear 
to be any consistency, other than that older code obviously is less 
likely to use the _d.h headers.

If we have a policy, then adding some annotations might also be a good 
way to describe it.

> 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?

I think the changes in main.c might have been my transcription error. 
They don't make sense.

> 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?

Ah, this is a deficiency in IWYU.  It keeps headers that provide 
function prototypes, but it doesn't keep headers that provide extern 
declarations of global variables.  I have filed an issue about that, and 
it looks like a fix might already be on the way.[0]

[0]: 
https://github.com/include-what-you-use/include-what-you-use/issues/1461

This issue also led me to discover -Wmissing-variable-declarations, 
about which I will post separately.

In the meantime, here is an updated patch with rebase and the above 
issues fixed.