Mailing-List: contact dev-help@perl.apache.org; run by ezmlm
Precedence: bulk
Message-ID: <3EB9E885.7010804@stason.org>
Date: Thu, 08 May 2003 15:17:57 +1000
From: Stas Bekman <stas@stason.org>
Organization: Hope, Humanized
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1) Gecko/20020826
MIME-Version: 1.0
To: Geoffrey Young <geoff@modperlcookbook.org>
Cc: dev@perl.apache.org
Subject: Re: DECLINE_CMD
References: <3EB9653C.7070301@modperlcookbook.org>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Geoffrey Young wrote:
> hi all
> 
>   I've been playing around with directive handlers and have found a 
> problem with DECLINE_CMD, namely that it doesn't exist within 
> Apache::Const.
> 
>   unfortunately, implementing DECLINE_CMD is not as easy as just adding 
> it to the constants map - DECLINE_CMD is the only constant in Apache/APR 
> that isn't an integer, and all the autogeneration/newCONSTSUB stuff is 
> geared toward creating integer constants.
> 
>   the below patch works and seems a decent way to do it (now we have a 
> routine for character-based constants in case there end up being 
> others), but somebody else might have a better idea.

I think I do have such idea ;)

I think your implementation is fine, however this:

  > +    else if (strnEQ(name, "DECLINE_CMD", 11)) {

adds an unnecessary string comparison in more than 50% of times.

I think there is a much cleaner solution. Fix ModPerl::Code to make
modperl_constants_lookup_apr and modperl_constants_lookup_apache return SV. 
Voila, now replace all current returns:

   return Foo;

with:

   return newSViv(Foo);

and add a special case for "DECLINE_CMD", which will do:

   return newSVpvn(DECLINE_CMD, 11);

Does it sound good?

BTW, why do we have the same generated files in two places?

grep -Ilr modperl_constants_lookup_apr src xs src/modules/perl/modperl_const.c 
| grep '\.c$'
src/modules/perl/modperl_const.c
src/modules/perl/modperl_constants.c
xs/ModPerl/Const/modperl_const.c
xs/ModPerl/Const/modperl_constants.c

Also I'd suggest to put this sub-test in t/apache/constants.t? since you just 
compare the string, without testing the functionality. Let's keep this 
particular test focused on perlmodule testing.

However a new test verifying the actual use of Apache::DECLINE_CMD would be 
useful.

[...]

> Index: t/response/TestDirective/perlloadmodule.pm
> ===================================================================
> RCS file: 
> /home/cvspublic/modperl-2.0/t/response/TestDirective/perlloadmodule.pm,v

[...]

> +
> +    ok t_cmp("\x07\x08", Apache::DECLINE_CMD);

Finally check: lib/Apache/ParseSource.pm (uncomment DECLINE_CMD there?)

And here is another missing constant, which is a string as well, (which will 
make your code even less efficient)

#define DIR_MAGIC_TYPE "httpd/unix-directory"

it's now commented out in lib/Apache/ParseSource.pm

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org