Re: PATCH: CITEXT 2.0

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