Re: PATCH: CITEXT 2.0

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Jul 2, 2008, at 09:13, Zdenek Kotala wrote:
>> 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.

Note that this sort of stuff will mostly be fixed by pg_indent,
whether or not David does anything about it. But certainly
conforming to the project style to begin with will cause less
pain to reviewers' eyeballs.

> Um, are you referring to this (at the top):

> +// PostgreSQL 8.2 Magic.
> +#ifdef PG_MODULE_MAGIC
> +PG_MODULE_MAGIC;
> +#endif

Here however is an outright bug: we do not allow // comments, because we
still support ANSI-spec compilers that don't recognize them.

> Yeah, I'm a bit confused by this. I followed what's in varlena.c on
> this. The comparison functions are documented supposed to return 1, 0,
> or -1, which of course would be covered by int. But varstr_cmp(),
> which ultimately returns the value, returns all kinds of numbers. So,
> perhaps someone could say what it's *supposed* to be?

btree cmp functions are allowed to return int32 negative, zero, or
positive. There is *not* any requirement that negative or positive
results be exactly -1 or +1. However, if you are comparing values
that are int32 or wider then you can't just return their difference
because it might overflow.

>> 3) citext_larger, citext_smaller function have memory leak. You
>> don't call PG_FREE_IF_COPY on unused text.

> These I also duplicated from varlena.c, and I asked about a potential
> memory leak in a previous email. If there's a leak here, would there
> not also be a leak in varlena.c?

The "leak" is irrelevant for larger/smaller. The only place where it's
actually useful to do PG_FREE_IF_COPY is in a btree or hash index
support function. In other cases you can assume that you're being
called in a memory context that's too short-lived for it to matter.

>> 5) There are several commented out lines in CREATE OPERATOR
>> 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.

http://developer.postgresql.org/pgdocs/postgres/xoper-optimization.html

regards, tom lane