Re: Glib Regex

[Havoc Pennington <hp redhat com>]

Hi,

Some random nitpicks, Owen is a big Perl/regexp fan so I'm sure he'll
have some more interesting things to say.

 - glib convention would make the flags an enum, not a set of macros

 - you have the same problem I was having with popt - you need
   the pcre #define constants in the header file. I think pcre.h
   needs to get kicked out of the header. 

   Perhaps to do this: create an enum in the header using glib
   conventions (Flags are 1 << 0, 1 << 1, 1 << 2, etc.); in the
   implementation, have some kind of translation table that converts
   to the PCRE #defines. The only other option I can think of is
   cut-and-paste.

   Your translation table could be indexed by the bit which is set (1
   << 0 means array index 0, 1 << 1 means array index 1, etc.) Maybe
   that's a lame idea, it's just a thought. :-)

 - legal nitpick - you need to include the full text of the licenses
   you mention

 - shouldn't g_regex_match() etc. take  a const gchar*? do they 
   modify the string?

 - I wouldn't include both list and slist variants of things that
   return a list. Just pick one. 

 - I'm not sure I'd include both array and list either; I think I
   might like the array more, but I don't know. If you do an array,
   probably the strings in the array should be allocated and the array
   should be NULL terminated so g_strfreev() works.

 - in the implementation, when you have a list of ints, you should 
   use GPOINTER_TO_INT and GINT_TO_POINTER to convert to/from 
   gpointer

 - Many of these functions violate the glib convention that 
   gchar* is returned as allocated memory, I think (unless
   pcre allocates the memory, in which case they violate the 
   requirement that memory be allocated with g_malloc()).
   This may be worth it for efficiency reasons.

 - in split_next() you use malloc instead of g_malloc

 - g_regex_optimize docs are above g_regex_study in the implementation

 - You have a comment asking if you should take a len parameter for
   strings; I think so, it's useful to be able to handle strings with
   nul bytes in them. Look at g_string_append and g_string_append_len
   in gstring.c in glib 1.3; basically the convention is to have two
   functions, and len can be -1 to indicate "length unknown, do a
   strlen()".  Then you implement plain _match() in terms of
   _match_len().

 - g_regex_clear() or g_regex_reset() seem like better names than 
   _reuse()

 - since every function takes the char* being matched, why can't you
   automatically reset the regex if the string changes, thus
   eliminating the _reuse() function?

 - if GREGEX_NULL means the string was NULL, this should be 
   a g_return_if_fail() not an error because it's a programmer
   mistake. 
  
 - in general you're missing g_return_if_fail() on all entry points

 - in g_regex_replace_eval() you free() a string from g_regex_match(),
   this will need to be g_free() and g_regex_match() should return 
   a string from g_malloc()

 - in the same function you don't free the string from the user
   callback, probably that string should be required to come from
   g_malloc(), otherwise users will have to use static buffers

 - I doubt the error messages are internationalized; if they are
   supposed to be for programmers only, they should be g_warning() and
   g_return_if_fail() not errors; if they're supposed to be
   potentially for users, they really should be internationalized. Can
   probably live with this for the first cut, but it is a bit wrong.

Anyway, looks good overall. Very simple to use, quite nice. And with
docs!

Havoc