After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 482319 - Switch from PCRE to GRegex
Switch from PCRE to GRegex
Status: RESOLVED FIXED
Product: libgoffice
Classification: Other
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: Jody Goldberg
Jody Goldberg
Depends on: 482313
Blocks:
 
 
Reported: 2007-10-01 18:02 UTC by Morten Welinder
Modified: 2008-04-26 01:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (14.78 KB, patch)
2008-04-23 03:08 UTC, Hiroyuki Ikezoe
none Details | Review
revised patch (18.91 KB, patch)
2008-04-25 00:40 UTC, Hiroyuki Ikezoe
none Details | Review

Description Morten Welinder 2007-10-01 18:02:40 UTC
We should be able to drop the pcre dependency, but glib needs work.
Comment 1 Jody Goldberg 2008-02-24 16:35:03 UTC
What do we need in glib to make this feasible for 1.8 ?
Comment 2 Jody Goldberg 2008-02-24 16:35:17 UTC
s/1.8/1.10/
Comment 3 Morten Welinder 2008-02-24 17:02:05 UTC
Get the fixed glib deployed, I guess.  I would think that we can do
"use glib if fixed" right now.
Comment 4 Hiroyuki Ikezoe 2008-04-23 03:08:21 UTC
Created attachment 109736 [details] [review]
proposed patch

NOTE: 
Some error codes of go_regcomp() slightly changed since some error codes are dropped in GRegex. So application using GORegexp may receive different error messages.
Comment 5 Morten Welinder 2008-04-23 13:04:52 UTC
Looks generally good, although I don't think we can require anything near
glib 2.16 yet.  Latest OpenSUSE comes with something like 2.12, give or take.
I imagine RedHat is similar.  Hence we should check for something like
G_REGEX_ERROR_STRAY_BACKSLASH and use glib if it is present and pcre
otherwise.

I am not worried over adjusting error codes.

What kinds of tests has this been through?
Comment 6 Hiroyuki Ikezoe 2008-04-23 23:18:05 UTC
(In reply to comment #5)
> Looks generally good, although I don't think we can require anything near
> glib 2.16 yet.  Latest OpenSUSE comes with something like 2.12, give or take.
> I imagine RedHat is similar.  Hence we should check for something like
> G_REGEX_ERROR_STRAY_BACKSLASH and use glib if it is present and pcre
> otherwise.

OK. I will revise the patch tomorrow. 

> What kinds of tests has this been through?

Loading samples/excel/datefuns.xls in gnumeric with Gnumeric.

Yes, I would like to add more test cases for goffice. Can I use GTester in GLib?

To tell the truth, I am a developer an Unit Testing Framework, Cutter[1] by name.
I would like to use it if you'd like.

Thank you,

[1] http://cutter.sourceforge.net/

Comment 7 Jody Goldberg 2008-04-24 00:00:23 UTC
We certainly wouldn't frown on a unit testing framework.  How does cutter compare with GTester ?
Comment 8 Morten Welinder 2008-04-24 13:00:35 UTC
Just make sure the test environment doesn't add some new requirement,
i.e., make sure it defaults to doing nothing if GTester/cutter/whatever
is not available.
Comment 9 Hiroyuki Ikezoe 2008-04-25 00:40:06 UTC
Created attachment 109873 [details] [review]
revised patch

Enclose huge ifdef blocks for readability.
Comment 10 Hiroyuki Ikezoe 2008-04-25 01:00:52 UTC
(In reply to comment #7)
> We certainly wouldn't frown on a unit testing framework.  How does cutter
> compare with GTester ?

These are my perspective. :-) 

== Cutter

* Easy to write (No need to register each test explicitly)
* Fast (One process per test suite, each test case can be run on multithread)
* Output stack trace when crashed

== GTester

* Supplied with GLib.
* Relatively slow (One process per test case)

I will post sample tests respectively.
Comment 11 Hiroyuki Ikezoe 2008-04-25 01:01:42 UTC
(In reply to comment #8)
> Just make sure the test environment doesn't add some new requirement,
> i.e., make sure it defaults to doing nothing if GTester/cutter/whatever
> is not available.

Yes, I will do so.

Comment 12 Hiroyuki Ikezoe 2008-04-25 07:11:47 UTC
I opened a new bug about test. BUG #529832.

Sorry for redundant information.
Comment 13 Morten Welinder 2008-04-26 01:24:47 UTC
Committed.  There were a number of problems:

1. _STRA_ --> _STRAY_

2. Leaked match_info in the no-match case.

3. Overwrote memory with extra matches.

4. Confused parentheses and multiple matches.


The GUI test case is Gnumeric's search-and-replace.