Re: patch adding new regexp functions

On Sat, 10 Feb 2007, Neil Conway wrote:

> On Fri, 2007-02-09 at 16:33 -0800, Jeremy Drake wrote:
> > Here is a new version of the patch which eliminates the doing_srf stuff.
>
> * C89 require constant-sized stack allocated arrays, so the coding in
> perform_regexp_matches() is non-portable.

I thought that was the case, but I had seen some other code doing this.
Turns out it was in the fulldisjunctions pgfoundry project.  I replaced
them with palloc'd memory.

> * I'm not clear about the control flow in regexp_matches() and
> regexp_split(). Presumably it's not possible for the call_cntr to
> actually exceed max_calls, so the error message in these cases should be
> elog(ERROR), not ereport (the former is for "shouldn't happen" bug
> scenarios, the latter is for user-facing errors). Can you describe the
> logic here (e.g. via comments) a bit more?

I added some comments, and changed to using elog instead of ereport.

> * The logic in regexp_split (incremented_offset, first_match, etc.) is
> pretty ugly and hard to follow. The "if" condition on line 1037 is
> particularly objectionable. Again, ISTM there should be a cleaner way to
> do this.

That if condition was very difficult to get right ;)  I added a bunch of
comments, and switched the logic around with a continue so it is much more
obvious what is happening there.  Incidentally, that if condition being
incorrect is what results in call_cntr exceeding max_calls :)

>
> * Try to keep lines to 80 characters or fewer. If the code is wandering
> off the right side of the screen all the time, that might be a hint that
> it needs simplification.
>
> Attached is a cleaned up version of your patch -- various improvements
> throughout, but mostly cosmetic stuff. Do you want to take a look at the
> above?
>
> -Neil
>
>

--
If God had meant for us to be naked, we would have been born that way.