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
