David E. Wheeler napsal(a):
> Howdy,
Howdy
> Please find attached a patch adding a locale-aware, case-insensitive
> text type, called citext, as a contrib module. A few notes:
I went through your code and I have following comments/questions:
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)
Other seems to me OK (remove 8.2 version mention at the bottom)
2) citextcmp function returns int but citext_cmp returns int32. I think
citextcmp should returns int32 as well.
3) citext_larger, citext_smaller function have memory leak. You don't call
PG_FREE_IF_COPY on unused text.
I'm not sure if return value in these function should be duplicated or
not.
4) Operator = citext_eq is not correct. See comment
http://doxygen.postgresql.org/varlena_8c.html#8621d064d14f259c594e4df3c1a64cac
There must be difference between equality and collation for example in
Czech
language 'láska' and 'laská' are different word it means that 'láska'
!=
'laská'. But there is no difference in collation order. See Unicode
Universal
Collation Algorithm for detail.
5) There are several commented out lines in CREATE OPERATOR statement
mostly
related to NEGATOR. Is there some reason for that? Also OPERATOR || has
probably
wrong negator.
6) You use pgTAP for unit test. It is something what should be widely
discussed.
It is new approach and I'm not correct person to say if it good or not.
I see there two problems:
At first point there is not clear licensing
(https://svn.kineticode.com/pgtap/trunk/README.pgtap).
And, It should be implemented as a general framework by my opinion not
only as a
part of citext contrib module.
Please, let me know when you will have updated code and I will start deep
testing.
With regards Zdenek
--
Sent via pgsql-hackers mailing list (pgsql-hackers@[EMAIL PROTECTED]
)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
|
