From: | Neil Conway <neilc(at)samurai(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Gevik Babakhani <pgdev(at)xs4all(dot)nl>, pgsql-patches <pgsql-patches(at)postgresql(dot)org> |
Subject: | Re: uuid patch 3.0 (8.3devel) |
Date: | 2007-01-28 16:20:09 |
Message-ID: | 1170001209.29138.33.camel@localhost.localdomain |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2007-01-28 16:40:06 | Re: uuid patch 3.0 (8.3devel) |
Previous Message | Gevik Babakhani | 2007-01-28 15:37:37 | Re: uuid patch 3.0 (8.3devel) |