perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philippe M. Chiasson" <go...@ectoplasm.org>
Subject Re: Please help adding ModPerl::Interpreter
Date Wed, 10 Oct 2007 19:20:25 GMT
Torsten Foertsch wrote:
> Hi Geoff,
> 
> On Tuesday 09 October 2007 19:19, Geoffrey Young wrote:
>> if you're really confused, see the commits when I added Apache::MPM
>>
>>   http://marc.info/?l=apache-modperl-cvs&m=106978727408877&w=2
>>   http://marc.info/?l=apache-modperl-cvs&m=106978912311523&w=2
>>
>> the bulk of the main commit consists of the tests, but the interesting
>> changes are at the bottom.  if you just follow that pattern you should
>> be able to add the module of your choice.
> 
> I think I did it. Please have a look at the attached patch to see if it is all 
> that is needed to be done.

It looks pretty good, awesome!

> The patch is a bit edited to suppress other changes in xs/. So I don't think 
> it would apply to trunk without warnings.

It did apply clean for me.

The only question I have is wherever it's necessary to also expose the
following 2 items:

modperl_interp_pool_t * | IV
PerlInterpreter *       | IV

Especially in the current patch, what you get out will pretty much
be a useless blessed scalar you can't do anything with.

Apart that, looks great. At this point, I'd be happy to see it go in,
without documentation even, but at least with a minimal set of tests,
something like:

my $int = ModPerl::Interpreter->current;
ok($int);
is($int, 'ModPerl::Intereter');

ok($int->mip);
ok($int->refcnt);
[...]

To at least cover normal usage cases.

Good work !
------------------------------------------------------------------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/       m/gozer\@(apache|cpan|ectoplasm)\.org/


Mime
View raw message