Re: Unused header file inclusion

Hi,

On 2019-07-31 16:55:31 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Jul-31, Andres Freund wrote:
> >> * I think a lot of the interlinking stems from the bad idea to use
> >> typedef's everywhere. In contrast to structs they cannot be forward
> >> declared portably in our version of C. We should use a lot more struct
> >> forward declarations, and just not use the typedef.
> 
> > I don't know about that ... I think the problem is that we both declare
> > the typedef *and* define the struct in the same place.  If we were to
> > split those things to separate files, the required rebuilds would be
> > much less, I think, because changing a struct would no longer require
> > recompiles of files that merely pass those structs around (that's very
> > common for Node-derived structs).  Forward-declaring structs in
> > unrelated header files just because they need them, feels a bit like
> > cheating to me.
> 
> Yeah.  I seem to recall a proposal that nodes.h should contain
> 
>     typedef struct Foo Foo;
> 
> for every node type Foo, and then the other headers would just
> fill in the structs, and we could get rid of a lot of ad-hoc
> forward struct declarations and other hackery.

That to me just seems objectively worse. Now adding a new struct as a
minor implementation detail of some subsystem doesn't just require
recompiling the relevant files, but just about all of pg. And just about
every header would acquire a nodes.h include - there's still a lot of them
that today don't.

I don't understand why you guys consider forward declaring structs
ugly. It's what just about every other C project does. The only reason
it's sometimes problematic in postgres is that we "hide the pointer"
within some typedefs, making it not as obvious which type we're
referring to (because the struct usage will be struct FooData*, instead
of just Foo). But we also have confusion due to that in a lot of other
places, so I don't really buy that this is a significant issue.


Right now we really have weird dependencies between largely independent
subsystem. Some are partially because functions aren't always in the
right file, but it's also not often clear what the right one would
be. E.g. snapmgr.h depending on relcache.h (for
TransactionIdLimitedForOldSnapshots() having a Relation parameter), on
resowner.h (for RegisterSnapshotOnOwner()) is imo not good.  For one
they lead to a number of .c files that actually use functionality from
resowner.h to not have the necessary includes.  There's a lot of things
like that.

.oO(the fmgr.h include in snapmgr.h has been unnecessary since 352a24a1f9)


We could of course be super rigorous and have an accompanying
foo_structs.h or something for every foo.h. But that seems to add no
actual advantages, and makes things more complicated.


The only reason the explicit forward declaration is needed in the common
case of a 'struct foo*' parameter is that C has weird visibility rules
about the scope of forward declarations in paramters. If you instead
first have e.g. a function *return* type of that struct type, the
explicit forward declaration isn't even needed - it's visible
afterwards. But for parameters it's basically a *different* struct, that
cannot be referenced again. Note that in C++ the visibility routines are
more consistent, and you don't need an explicit forward declaration in
either case (I'd also be OK with requiring it in both cases, it's just
weird to only need them in one).

Greetings,

Andres Freund