On Sat, 2007-01-27 at 23:53 -0500, Tom Lane wrote:
> A few comments after a quick once-over, perhaps you caught all this
> already...
Much of it :) But thanks for the review.
> uuid.c header is missing $PostgreSQL$ line, so is uuid.h,
> copyright notice in the latter seems wrong too.
I think the copyright header on completely new code should just include
the current year, shouldn't it? I've just marked both files as
"Copyright (c) 2007 [PGDG]".
> Generally I like to put "local var = PG_GETARG()" declarations
> first in a function, not randomly mixed in with other declarations
> of local variables. Think of them as part of the function header.
> (Someday we might try to process them with some automatic script,
> too... so the less random stylistic variation, the better.)
Agreed on both points; of course, PG_GETARG_XXX(n) should also be
ordered in ascending order of "n".
> uuid.c contains some functions that are declared static and
> then defined without, please clean this up, and make sure
> it's not exporting anything it doesn't have to.
Notably, the uuid_t struct doesn't need to be exported.
> The regression test is probably expending a lot more cycles than are
> justified, eg what exactly is the point of the last part that inserts
> 32K random-data records? If it ever did show a failure we'd be unable
> to reproduce it, so please at least lose the use of now() and random().
I just removed the test, as it seemed unlikely to be helpful.
Patch applied with various stylistic changes (including all of Tom's
suggestions), catversion bumped.
-Neil
