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 ---------------------------(end of broadcast)--------------------------- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate