> Hmm, I haven't realized that it's used in _module.c as well. So my idea
> introduces a bit of an overhead to allocate the SV, when it only wants
> an int.
> But since this happens only during the server startup, I suppose it's
> not a problem.
that was my thought.
[snip]
>
> A few more comments below:
>
>> Index: lib/ModPerl/Code.pm
>
> [...]
>
>> $proto
>> {
>> + dTHX;
>
>
> It's going to be much more efficient to pass pTHX_ to the function.
ok, I'll try that. I really have no idea how threads work, I just figured
out that I needed something to get it to compile :)
>
>> + if (strEQ(name, "$name")) {
>> + return newSVpv($alias{$name},0);
>
>
> Use newSVpvn, you know the length already (faster).
well, then I need two if statements - one for DECLINE_CMD and one for
DIR_MAGIC_TYPE. I'm not sure that the added logic is worth the (one time
and slight) hit.
>
>> + }
>> +$ifdef[1]
>> +EOF
>> + }
>> + else {
>> + print $c_fh <<EOF;
>> $ifdef[0]
>> if (strEQ(name, "$name")) {
>> - return $alias{$name};
>> + return newSViv($alias{$name});
>
>
> Use strnEQ (faster), like the orig int functions do.
the original functions use strEQ - I didn't change that for the int ones and
only carried it over for the string ones.
> You don't really
> have to make special cases for these two constants. Just use the old
> generation code and put 'return SVpvn($alias{$name});' if perl sees
> these two constants, otherwise put 'return SViv($alias{$name});'
yeah :)
as per SViv vs newSViv, if we remove the newSV stuff then is it still save
to remove the newSV from val here:
> I think it should be:
>
> - newCONSTSUB(stash, (char *)name, newSViv(val));
> + newCONSTSUB(stash, (char *)name, val);
?
>
>> -typedef int (*constants_lookup)(const char *);
>> +typedef SV *(*constants_lookup)(const char *);
>
>
> So pTHX_ goes here:
>
> -typedef int (*constants_lookup)(const char *);
> +typedef SV *(*constants_lookup)(pTHX_ const char *);
>
> and aTHX_ in the caller...
>
>> + SvIV(modperl_constants_lookup_apache(SvPV(val,
>> len)));
>> }
>> }
>>
>> @@ -695,7 +695,7 @@
>> }
>> else {
>> cmd->req_override =
>> - modperl_constants_lookup_apache(SvPV(val, len));
>> + SvIV(modperl_constants_lookup_apache(SvPV(val,
>> len)));
>
>
> it's probably safe to use SvIVX here, but since this code is not
> run-time critical SvIV is just fine.
cool, thanks for the comments. I'll have (another) patch soon.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
|