On Jul 2, 2008, at 09:13, Zdenek Kotala wrote:
> I went through your code and I have following comments/questions:
Thanks. I'm on a short family vacation, so it will probably be
Monday=20=20
before I can submit a new patch. I got a few replies below, though.
> 1) formating
>
> Please, do not type space before asterix:
> char * lcstr, * rcstr -> char *lcstr, *rcstr
>
> and do not put extra space in a brackets
> citextcmp( left, right ) -> citextcmp(left, right)
Okay.
> Other seems to me OK (remove 8.2 version mention at the bottom)
Um, are you referring to this (at the top):
+// PostgreSQL 8.2 Magic.
+#ifdef PG_MODULE_MAGIC
+PG_MODULE_MAGIC;
+#endif
That's gotta be there (though I can of course remove the comment).
> 2) citextcmp function returns int but citext_cmp returns int32. I=20=20
> think citextcmp should returns int32 as well.
Yeah, I'm a bit confused by this. I followed what's in varlena.c on=20=20
this. The comparison functions are do***ented supposed to return 1, 0,=20=
=20
or -1, which of course would be covered by int. But varstr_cmp(),=20=20
which ultimately returns the value, returns all kinds of numbers.
So,=20=20
perhaps someone could say what it's *supposed* to be?
> 3) citext_larger, citext_smaller function have memory leak. You=20=20
> don't call PG_FREE_IF_COPY on unused text.
>
> I'm not sure if return value in these function should be
duplicated=20=20
> or not.
These I also duplicated from varlena.c, and I asked about a
potential=20=20
memory leak in a previous email. If there's a leak here, would there=20=20
not also be a leak in varlena.c?
> 4) Operator =3D citext_eq is not correct. See comment
http://doxygen.pos=
tgresql.org/varlena_8c.html#8621d064d14f259c594e4df3c1a64cac
So should citextcmp() call strncmp() instead of varst_cmp()? The=20=20
latter is what I saw in varlena.c.
> There must be difference between equality and collation for
example=20=20
> in Czech language 'l=E1ska' and 'lask=E1' are different word it
means=20=
=20
> that 'l=E1ska' !=3D 'lask=E1'. But there is no difference in
collation=20=
=20
> order. See Unicode Universal Collation Algorithm for detail.
I'll leave the collation stuff to the functions I call (*far* from
my=20=20
specialty), but I'll add a test for this and make sure it works as=20=20
expected. Um, although, with what collation should it be tested? The=20=20
tests I wrote assume en_US.UTF-8.
> 5) There are several commented out lines in CREATE OPERATOR=20=20
> statement mostly related to NEGATOR. Is there some reason for that?
I copied it from the original citext.sql. Not sure what effect it has.
> Also OPERATOR || has probably wrong negator.
Right, good catch.
> 6) You use pgTAP for unit test. It is something what should be=20=20
> widely discussed. It is new approach and I'm not correct person
to=20=20
> say if it good or not.
Sure. I created a pgFoundry project for it here:
http://pgtap.projects.postgresql.org/
> I see there two problems:
> At first point there is not clear licensing
(https://svn.kineticode.com/p=
gtap/trunk/README.pgtap=20
> ).
That's a standard revised BSD license.
> And, It should be implemented as a general framework by my opinion=20=20
> not only as a part of citext contrib module.
It is. I just put the file in citext so that the tests can use it.=20=20
It's distributed on pgFoundry and maintained at the SVN URL quoted
in=20=20
the file.
> Please, let me know when you will have updated code and I will start=20=
=20
> deep testing.
Will do.
Best,
David
--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@[EMAIL PROTECTED]
)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
|
