Libguestfs - Aug 2017 - Re: [PATCH 0/2] Add lightweight bindings for PCRE.

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

On Thu, Aug 03, 2017 at 06:08:24PM +0200, Pino Toscano
wrote:
> 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 ...
Fewer dependencies make the code easier for others to consume, and
that's especially true when (like oUnit) those dependencies add little
value.  (That's different from dependencies like, say, Augeas which
bring huge value.)

So by adding a test with asserts I'm gradually working towards
reducing -- or at least not increasing -- the amount of oUnit being
used.

Practically, someone who compiles libguestfs and runs the tests but
doesn't have oUnit installed will be able to run this test, whereas
the oUnit-based tests would be skipped.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org