On Sun, Jan 25, 2009 at 03:52:02PM +0000, Roger Leigh wrote:
> Many thanks for this suggestion; it has been incredibly useful.
No problem.
> I just have a few points I'd like to clarify. In
>
>
http://git.debian.org/?p=users/rleigh/sbuild.git;a=blob;f=db/debversion.cc;h=980786ecf7a3b7fb5769d04b0952af300723c3b9;hb=81fd39259953853632a7d0e2198cfc745d270fe3
>
> debversioncmp (lines 49-73), I'm duplicating text* to char* by hand.
> Is the text_to_cstring available internally also accessible by
> external modules (I didn't see it in any headers)?
A function like that exists, the "proper" way to do it is (untested):
DatumGetCString( DirectFunctionCall1(textout, TextGetDatum(foo) )
It's used in various places, in the unreleased 8.4 there will be
official functions like cstring_to_text and text_to_cstring. Many other
modules already declare stuff like this.
> After every PG_GETARG_TEXT_PP, I've called PG_FREE_IF_COPY before
> returning. However, I saw in citext (the behaviour of which I
> duplicated in debversion_smaller and debversion_larger (lines 221-246))
> that you *don't* use PG_FREE_IF_COPY before returning one of the two
> values. Is this not a potential memory leak? Don't you need to
> duplicate your chosen value, then free both of the temporary values?
Memory leaks are not an issue generally, everything you allocate gets
freed at the end of the statement, if not earlier. Normally you don't
manually free at all, however code that might be used by indexes is
somewhat of an exception, since they might be called often in a tight
loop. smaller/larger are not index functions, but the cmp function is.
Though your strings here are probably short enough you won't notice
either way.
> I also noticed that I couldn't include some postgres headers in
> my code (e.g. <access/builtins.h>) due to included headers
> <nodes/parsenodes.h> and <nodes/primnodes.h> using invalid C++ syntax
C++ incompatability has been noted before and patches were posted. I
don't remember right now what happened to them, check the archives. The
usual workarond is to split the C++ specific stuff into a seperate
file, but that's kind of ugly I agree.
Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.