[Libguestfs] [PATCH 0/2] Add lightweight bindings for PCRE.

[Pino Toscano]

On Wednesday, 2 August 2017 20:18:43 CEST Richard W.M. Jones wrote:
> On Wed, Aug 02, 2017 at 03:01:53PM +0200, Pino Toscano wrote:
> > On Wednesday, 2 August 2017 13:52:06 CEST Richard W.M. Jones wrote:
> > > I think you're suggesting this:
> > > 
> > >   let m = PCRE.exec re "ccaaaabb" in
> > >   if PCRE.matches m then (
> > >     let whole = PCRE.sub m 0 in
> > 
> > Not really, my suggestion was to have a separate object representing
> > the result of a regex match -- much like other language have in their
> > regex APIs.
> > 
> > OTOH, this solution LGTM as well: the result of the regex is not saved
> > in a thread-local variable, but directly in the same regex object, so
> > can be kept/used around, and it is GC'ed when not needed anymore.
> > If you could apply that change, that'd be a LGTM.
> 
> But the problem still is this makes the if/else chains more awkward.
> 
> What's really wrong with the thread-local variable?  It only stores a
> single string and a vector of integers per thread (at most).  The
> final string/vector is reliably freed on exit - I know that because
> I've checked it with valgrind.

I've no doubts that the patch cleanes the memory correctly.  I simply
not in favour of global variables in complex/big contests, since they
can be used by every place (thus hard to trace), their data linger
longer than the actual usage (like in this case), and the possible
(sometimes subtle) iterations among them makes the code harder to
follow.

But I guess I have no chance to change the current patch (even adding
a test using asserts, instead of oUnit, for which I still did not get
a reason why a new test using it would be unacceptable), so ...

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170803/f45af0d0/attachment.sig>