Hi,
On 2019-01-14 15:36:14 -0300, Alvaro Herrera wrote:
> 0001 -- looks reasonable. One hunk in executor.h changes LockTupleMode
> to "enum LockTupleMode", but there's no need for that.
Oh, that escaped from an earlier version where I briefly forgot that one
cannot portably forward-declare enums.
> AFAIK the only reason to have the struct FooBarData vs. FooBar (ptr)
> split is so that it's possible to refer to structs without having
> the full struct definition. I think changing uses of FooBar to "struct
> FooBarData *" defeats the whole purpose -- it becomes pointless noise,
> confusing the reader for no gain. I've long considered that the struct
> definitions should appear in "internal" headers (such as
> htup_details.h), separate from the pointer typedefs, so that it is the
> forward struct declarations (and the pointer typedefs, where there are
> any) that are part of the exposed API for each module, and not the
> struct definitions.
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.
If we were in C11 we could just forward declare the pointer hiding
typedefs in multiple places, and be done with that. But before C11
redundant typedefs aren't allowed. With the C99 move I'm however not
sure if there's any surviving supported compiler that doesn't allow
redundant typedefs as an extension.
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.
> I think MissingPtr is a terrible name. Can we change that while at
> this?
Indeed. I'd just remove the typedef.
Greetings,
Andres Freund
