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.