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
