Re: transformExpr() refactor

From: Neil Conway <neilc(at)samurai(dot)com>
To: pgsql-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: transformExpr() refactor
Date: 2005-01-18 05:15:57
Message-ID: 1106025357.22946.134.camel@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

On Thu, 2004-10-28 at 16:49 +1000, Neil Conway wrote:
> This patch refactors transformExpr(): rather than being a monsterous 900
> line function, it breaks it up into numerous sub-functions that are
> invoked by transformExpr() for individual expression types, in the style
> of transformStmt().

I still think this patch is worth applying. Sadly, I will need to
basically rewrite it due to code drift. I'm happy to do that, although
I'd like to resolve whether we want to accept it or not. Tom and Bruce
objected when I posted it originally, although I don't think we reached
a conclusion.

Why I think the patch is a good idea: 900 line functions are almost
universally bad (in fact, I'd be tempted to remove the "almost"). A 900
line function that divides distinct functionality into different
branches of a "switch" statement isn't _that_ evil, but it is still a
large hunk of code for someone to understand as a single, monolithic
unit. Because each branch of the "switch" statement is independent, we
can trivially split each branch off into a function -- each branch does
something distinct, so it ought to be a distinct function. That means
the independence of each branch of the "switch" statement is guaranteed
(the function can't modify a local variable in the calling function, for
example), and it means that the code is conceptually easier to read.
With the present layout,

It does mean that you'll need to jump to the function definition if you
want to see what a particular branch of the "switch" statement does. But
(a) use tags, it's not tough (b) this is a _good_ thing. If I want to
understand what the parent function does (transformExpr(), the one with
the "switch"), I don't want to have to grok through a 700 line "switch"
statement. If each branch of the switch just invokes a function, the
intent of transformExpr() is perfectly clear.

For refresh everyone's memory, I've attached the original version of the
patch. It won't apply cleanly to current sources, but it should apply to
HEAD as of approximately Oct. 28, 2004.

-Neil

Attachment Content-Type Size
transform_expr_refactor-2.patch text/x-patch 49.0 KB

In response to

Responses

Browse pgsql-patches by date

  From Date Subject
Next Message Neil Conway 2005-01-18 05:43:40 Re: rtree: improve performance, tuple killing
Previous Message Neil Conway 2005-01-18 04:57:37 WIP: pl/pgsql cleanup