Thread: Sorting case branches in outfuncs.c/outNode alphabetically

Sorting case branches in outfuncs.c/outNode alphabetically

From
Fedir Panasenko
Date:
Hi all,

outfuncs.c contains a switch statement responsible for choosing
serialization function per node type here:
https://github.com/postgres/postgres/blob/master/src/backend/nodes/outfuncs.c#L3711
It spans over >650LOC and is quite unreadable, requiring using search
or code analysis tools for pretty much anything.

I'd like to sort these case branches alphabetically and I'd like to
get some input on that prior to submitting a patch. Obvious benefit
would be increase in readability, with the downside of somewhat
messing up the git history.

(readfuncs.c contains a similar construct for deserializing nodes, but
that one is if...else based as opposed to switch, so order there might
have performance implications -> I'd reserve that topic for separate
discussion).

---
Best regards,
Fedir



Re: Sorting case branches in outfuncs.c/outNode alphabetically

From
Tom Lane
Date:
Fedir Panasenko <fpanasenko@gmail.com> writes:
> outfuncs.c contains a switch statement responsible for choosing
> serialization function per node type here:
> https://github.com/postgres/postgres/blob/master/src/backend/nodes/outfuncs.c#L3711

Why are you concerned about outfuncs.c in particular?  Its sibling files
(copyfuncs, equalfuncs, etc) have much the same structure.

> It spans over >650LOC and is quite unreadable, requiring using search
> or code analysis tools for pretty much anything.

But why exactly do you need to read it?  It's boring boilerplate.

> I'd like to sort these case branches alphabetically and I'd like to
> get some input on that prior to submitting a patch.

I'd be a strong -1 for alphabetical sort.  To my mind, the entries
here, and in other similar places, should match the order in which the
struct types are declared in src/include/nodes/*nodes.h.  And those
are not sorted alphabetically, but (more or less) by functionality.
I would *definitely* not favor a patch that arbitrarily re-orders
those header files alphabetically.

Now, IIRC the ordering in the backend/nodes/*.c files is not always
a perfect match to the headers.  I'd be good with a patch that makes
them line up better.  But TBH, that is just neatnik-ism; I still don't
see how it makes any interesting difference to readability.

Keep in mind also that various people have shown interest in
auto-generating the backend/nodes/*.c files from the header
declarations, in which case this discussion would be moot.

> (readfuncs.c contains a similar construct for deserializing nodes, but
> that one is if...else based as opposed to switch, so order there might
> have performance implications -> I'd reserve that topic for separate
> discussion).

Yeah, I keep wondering when that structure is going to become a
noticeable performance problem.  There's little reason to think that
we've ordered the node types by frequency there.  At some point it might
make sense to convert readfuncs' lookup logic into, say, a perfect hash
table (cf src/tools/PerfectHash.pm).  I'd certainly think that that
would be a more useful activity than arguing over the switch order.

            regards, tom lane