Re: Acceptable/Best formatting of callbacks (for pluggable storage)

Hi,

On 2019-01-11 10:32:03 -0500, Robert Haas wrote:
> On Thu, Jan 10, 2019 at 11:45 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > void (*relation_set_new_filenode) (Relation relation,
> > char persistence,
> > TransactionId *freezeXid,
> > MultiXactId *minmulti);
>
> Honestly, I don't see the problem with that, really.

It's just hard to read if there's a lot of callbacks defined, the more
accurate the name, the more deeply indented. Obviously that's always a
concern and thing to balance, but the added indentation due to the
whitespace, and the parens, * and whitespace between ) ( make it worse.

> But you could
> also use the style that is used in fdwapi.h, where we have a typedef
> for each callback first, and then the actual structure just declares a
> function pointer of each time. That saves a bit of horizontal space
> and might look a little nicer.

It's what the patch did at first. It doesn't save much space, because
the indentation due to the typedef at the start of the line is about as
much as defining in the struct adds, and we often add a _function
suffix. It additionally adds a fair bit of mental overhead - there's
another set of names that one needs to keep track of, figuring out what
a callback means requires looking in an additional place. I found that
removing that indirection made for a significantly more pleasant
experience working on the patchset.

Just as an example of why I think this isn't great:

typedef Size (*EstimateDSMForeignScan_function) (ForeignScanState *node,
ParallelContext *pcxt);
typedef void (*InitializeDSMForeignScan_function) (ForeignScanState *node,
ParallelContext *pcxt,
void *coordinate);
typedef void (*ReInitializeDSMForeignScan_function) (ForeignScanState *node,
ParallelContext *pcxt,
void *coordinate);
typedef void (*InitializeWorkerForeignScan_function) (ForeignScanState *node,
shm_toc *toc,
void *coordinate);
typedef void (*ShutdownForeignScan_function) (ForeignScanState *node);
typedef bool (*IsForeignScanParallelSafe_function) (PlannerInfo *root,
RelOptInfo *rel,
RangeTblEntry *rte);
typedef List *(*ReparameterizeForeignPathByChild_function) (PlannerInfo *root,
List *fdw_private,
RelOptInfo *child_rel);

that's a lot of indentation variability in a small amount of space - I
find it noticably slower to mentally parse due to that. Compare that
with:

typedef Size (*EstimateDSMForeignScan_function)
(ForeignScanState *node,
ParallelContext *pcxt);

typedef void (*InitializeDSMForeignScan_function)
(ParallelContext *pcxt,
void *coordinate);

typedef void (*ReInitializeDSMForeignScan_function)
(ForeignScanState *node,
ParallelContext *pcxt,
void *coordinate);

typedef void (*InitializeWorkerForeignScan_function)
(ForeignScanState *node,
shm_toc *toc,
void *coordinate);

typedef void (*ShutdownForeignScan_function)
(ForeignScanState *node);

typedef bool (*IsForeignScanParallelSafe_function)
(PlannerInfo *root,
RelOptInfo *rel,
RangeTblEntry *rte);

typedef List *(*ReparameterizeForeignPathByChild_function)
(PlannerInfo *root,
List *fdw_private,
RelOptInfo *child_rel);

I find the second formatting considerably easier to read, albeit not for
the first few seconds.

Greetings,

Andres Freund