Re: [PATCHES] Text <-> C string

On 20/03/2008, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I started to look at applying this patch and immediately decided that
> I didn't like these names --- it's exceeding un-obvious which direction
> of conversion any one of the functions performs. Seems like every time
> you wanted to call one, you'd be going back to look at the source code
> to remember which to use.
>

That's a fair criticism. I wanted to make the function names as
compact as possible, but your comment about the directionality of the
functions rings true.

> What do people think of text_to_cstring? Or should we go with
> TextPGetCString for consistency with the Datum-whacking macros? (I seem
> to recall having argued against the latter idea, but am reconsidering.)
> Or something else?
>

Your original argument against FooGetBar was that it would be *too*
consistent with the Datum macros, leading people to think that these
functions actually were macros.

As long as we don't want people getting confused about the
function/macro distinction, that argument still makes sense to me, and
I'd be more inclined towards the foo_to_bar() convention.

> BTW, I suspect that the _limit functions are mostly useless ---
> a quick look through the patch did not suggest that any of the places
> using them really needed a limit. The point of, for instance,
> TZ_STRLEN_MAX is to be able to use fixed-size local buffers, and
> if you're going to pay for a palloc anyway then you might as well
> forget it.

What about callers like dotrim() in oracle_compat.c, which only want
to copy characters from the source string up to a particular length?
Doesn't that indicate a legitimate requirement for a
cstring_to_text_limit() (the call site was palloc'ing the text value
anyway)?

On the other hand, we do have those call sites (TZ_STRLEN_MAX is a
good example) where the caller just wanted to use a local buffer. In
which case your strlcpy-equivalent function would probably be the
right thing to offer.

> (There might be some value in a strlcpy equivalent that
> copies from a text datum into a limited-size caller-supplied buffer,
> but that's not what we've got here.)
>

Perhaps we keep cstring_to_text_limit(), but make
text_to_cstring_limit() behave like strlcpy() instead?

One of the questions in the original patch submission was whether it
would be worth changing all those DirectFunctionCall(textin) and
(textout) calls to use the new functions. Is it worthwhile avoiding
the fmgr overhead?

Thanks for taking the time to review my patch and provide this
feedback. I'm happy to send in an updated version once we settle on
the naming convention for the functions.

Last time I looked, the codebase had shifted quite a bit since I
originally wrote the patch. So it probably needs some work to apply
cleanly on the latest sources anyway.

Regards,
BJ