Re: Reducing header interdependencies around heapam.h et al. - Mailing list pgsql-hackers

Hi,

On 2019-01-14 17:55:44 -0300, Alvaro Herrera wrote:
> On 2019-Jan-14, Andres Freund wrote:
>
> > I think the whole pointer hiding game that we play is a really really
> > bad idea. We should just about *never* have typedefs that hide the fact
> > that something is a pointer. But it's hard to go away from that for
> > legacy reasons.
> >
> > The problem with your approach is that it's *eminently* reasonable to
> > want to forward declare a struct in multiple places. Otherwise you end
> > up in issues where you include headers like heapam.h solely for a
> > typedef, which obviously doesn't make a ton of sense.
>
> Well, my point is that in an ideal world we would have a header where
> the struct is declared once in a very lean header, which doesn't include
> almost anything else, so you can include it into other headers
> liberally.  Then the struct definitions are any another (heavy) header,
> which *does* need to include lots of other stuff in order to be able to
> define the structs fully, and would be #included very sparingly, only in
> the few .c files that really needed it.

> For example, I would split up execnodes.h so that *only* the
> typedef/struct declarations are there, and *no* struct definition.  Then
> that header can be #included in other headers that need those to declare
> functions -- no problem.  Another header (say execstructs.h, whatever)
> would contain the struct definition and would only be used by executor
> .c files.  So when a struct changes, only the executor is recompiled;
> the rest of the source doesn't care, because execnodes.h (the struct
> decls) hasn't changed.

It's surely better than the current state, but it still requires
recompiling everything in a more cases than necessary.


> This may be too disruptive.  I'm not suggesting that you do things this
> way, only describing my ideal world.

It'd probably doable by leaving execnodes.h as the heavyweight nodes,
and execnodetypes.h as the lightweight one, and including the latter
from the former. And then moving users of execnodes over to
execnodetypes.


> Your idea of "liberally forward-declaring structs in multiple places"
> seems like you don't *have* the lean header at all (only the heavy one
> with all the struct definitions), and instead you distribute bits and
> pieces of the lean header randomly to the places that need it.  It's
> repetitive.  It gets the job done, but it's not *clean*.

I'm not really buying the repetitiveness bit - it's really primarily
adding 'struct ' prefix, and sometimes adding a 'Data *' postfix. That's
not a lot of duplication. When used in structs there's no need to even
add an explicit 'struct <name>;' forward declaration, that's only needed
for function parameters.  And afterwards there's a lot less entanglement
- no need to recompile every file just because a new node type has been
added etc.


> > Given the fact that including headers just for a typedef is frequent
> > overkill, hiding the typedef in a separate header has basically no
> > gain. I also don't quite understand why using a forward declaration with
> > struct in the name is that confusing, especially when it only happens in
> > the header.
>
> Oh, that's not the confusing part -- that's just repetitive, nothing
> more.  What's confusing (IMO) is having two names for the same struct
> (one pointer and one full struct).  It'd be useful if it was used the
> way I describe above.  But that's the settled project style, so I don't
> have any hopes that it'll ever be changed.

Not within a few days, but we probably can do it over time...


> Anyway, I'm not objecting to your patch ... I just don't want it on
> record that I'm in love with the current situation :-)

Cool, I've pushed these now. I'll rebase my patch to split
(heap|reation)_(open|close)(rv)? patch out of heapam.[ch] now. Brr.

Greetings,

Andres Freund