You are viewing a plain text version of this content. The canonical link for it is here.
Posted to modperl@perl.apache.org by Nathan Byrd <na...@byrd.net> on 2003/02/03 05:21:17 UTC
[patch] Changes to RegistryCooker for subclassing
All,
To begin with, should proposed mod_perl patches go to
dev@perl.apache.org? The documentation seemed a little unclear in this
case (I decided to play it safe since I didn't run across any messages
on the dev list from outside developers.)
When I was converting Apache::PAR to work with mod_perl 2.x, one problem
I had was with the way in which ModPerl::RegistryCooker stores member
data. Any subclass of ModPerl::RegistryCooker (in my case, for
Apache::PAR::RegistryCooker) that need to store their own module data
have a problem - they need to pick an array element to store their data
in. Because of the way in which ModPerl::RegistryCooker works
currently, that means hardcoding an array index (because not all array
members are created in new or init). Thus, in
Apache::PAR::RegistryCooker I have a line similar to the following:
use constant PARDATA => 8;
This is not optimal, especially since this has already changed in the
CVS version of RegistryCooker since I started working on it - luckily,
to less members, not more :-)
I propose a change to RegistryCooker.pm so that member data is always
defined in the init sub, so that I could change the above line to
something more like:
use constant PARDATA => -1;
and in my handler, push a new element on after new has been called.
This would keep future changes to the RegistryCooker script from
potentially breaking other modules which must store their own data as
well.
Below is a (small) patch to the CVS version of RegistryCooker.pm to do
this. Down side is that if new member data is added, it would then also
need to be added to the init sub.
If you have any questions, please let me know.
*** RegistryCooker.pm.bak Sun Feb 2 21:00:59 2003
--- RegistryCooker.pm Sun Feb 2 21:10:51 2003
***************
*** 107,124 ****
--- 107,128 ----
#########################################################################
# func: init
# dflt: init
# desc: initializes the data object's fields: REQ FILENAME URI
+ # also declares other members not yet used
# args: $r - Apache::Request object
# rtrn: nothing
#########################################################################
sub init {
$_[0]->[REQ] = $_[1];
$_[0]->[URI] = $_[1]->uri;
$_[0]->[FILENAME] = $_[1]->filename;
+ $_[0]->[MTIME] = undef;
+ $_[0]->[PACKAGE] = undef;
+ $_[0]->[CODE] = undef;
}
#########################################################################
# func: handler
# dflt: handler
Thanks,
--
Nathan Byrd <na...@byrd.net>
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Sat, 2003-03-01 at 22:17, Stas Bekman wrote:
> Nathan, can you please do:
>
> cd modperl-2.0/ModPerl-Registry
> cvs diff -u lib/ModPerl > patch
>
> and send the patch as an attachment. Your original patch doesn't apply :(
>
> Thanks.
>
Stas,
Sorry about that, not sure what happened. Attached is a new version
using the above instructions. If you have any questions or problems
applying this, please let me know. Thanks,
--
Nathan Byrd <na...@byrd.net>
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Nathan, can you please do:
cd modperl-2.0/ModPerl-Registry
cvs diff -u lib/ModPerl > patch
and send the patch as an attachment. Your original patch doesn't apply :(
Thanks.
__________________________________________________________________
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
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Wed, 2003-02-26 at 00:39, Stas Bekman wrote:
> Nathan Byrd wrote:
> [...]
[...]
>
> This patch does nothing, but replaced array based object with hash based one,
> right? that's fine with me. Thank you.
>
Yep - almost seems anti-climatic, doesn't it? :-)
> re: No accessors provided. So if tomorrow I fancy to change some key names, I
> can't do that since it'll break sub-classes, right? What if we need to add a
> new key? this may clobber the sub-classes private keys?
Yeah, that's the downside to no accessors with this (deleting or
renaming keys could break sub-classes). Myself I think its probably
made up for by simplicity though, especially since if we added accessors
then it would then be the accessors that we couldn't rename or delete,
etc without breaking sub-classes.
> If so, at least I'd
> suggest the subclasses to take safety precautions and name their keys as
> whatever_unique_string_ + attribute, e.g. par_foo, par_bar?
Actually, what I did in my module was to take it a step further, and use
my fields like $self->{PARDATA}{foo}, $self->{PARDATA}{bar} etc so there
was only one key to worry about (not too worried about performance or
further sub-classing in my module which digs through zip files :-), but
fields like par_foo, par_bar, etc would work fine too.
Thanks,
> --
> Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Nathan Byrd wrote:
[...]
> Sorry for taking so long to get back with you. I've finally gotten a
> chance to start looking at this again. Making the module changes with
> hashes does seem to simplify things. As to performance - besides your
> point above about most of the work happening in the scripts, I believe
> that this shouldn't be worse than mod_perl 1.x anyway since it is also
> based on hash objects. I suppose most people probably avoid PerlRun or
> Registry as well if optimization is that important to them that they
> would actually care about the performance difference between an array
> based object and hash based object.
>
> Also, I followed your suggestion and skipped making accessors for each
> member. When I started implementing them it seemed a little excessive
> when using a hash based object.
>
> Below is patches to RegistryCooker and RegistryLoader to use hash based
> objects. The following is a patched and tested (passes make test)
> against the latest CVS. Please let me know what you think.
This patch does nothing, but replaced array based object with hash based one,
right? that's fine with me. Thank you.
re: No accessors provided. So if tomorrow I fancy to change some key names, I
can't do that since it'll break sub-classes, right? What if we need to add a
new key? this may clobber the sub-classes private keys? If so, at least I'd
suggest the subclasses to take safety precautions and name their keys as
whatever_unique_string_ + attribute, e.g. par_foo, par_bar?
__________________________________________________________________
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
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Wed, 2003-02-12 at 00:23, Stas Bekman wrote:
> Nathan Byrd wrote:
> [...]
> > fields Pragma
> > -------------
> > Advantages:
> > * More straightforward approach
> > * Less code to add to module
> >
> > Disadvantages:
> > * Based on pseudo-hashes - may have problems with reloading, etc
> > * Unknown whether it changes performance (due to "typed lexical"
> > limitation of pragma)
> > * Forces developers to 'use base' and/or 'use fields' in sub-class, can
> > cause non-obvious error otherwise
> >
> > Array based
> > -----------
> > Advantages:
> > * Easy to change code to change base class implementation in the future
> > * Good encapsulation (ability to override methods in future without
> > breaking sub-classes)
> >
> > Disadvantages:
> > * Adds a custom non-standard extending technique
> > * Need to explain method in doco for adding sub-class data (via
> > "public"), vs. pointing the developer to an existing perl document.
> > * Doesn't work with multiple inheritance (probably not a big deal for
> > most RegistryCooker sub-class developers though)
>
> Thanks for the summary Nathan,
>
> Looking at it, I favor your original proposal, as then we have a complete
> control and any bugs are our own fault.
>
> Considering that we go with that approach. Are there any in-core modules that
> can be used for creating/managing accessors? I know there is a whole lot of
> them on CPAN, but we don't want to create extra dependencies.
>
> Another approach to simplify your original suggestion, is to move on to hash
> based objects. I doubt it'll be of any significant difference, since most work
> happens in the scripts themselves. I guess a benchmark will show better. But
> if we go with it, subclasses can simply bypass the accessors (we could just
> skip them) and work directly with the hash. Dunno, what's the best way to go.
>
Stas,
Sorry for taking so long to get back with you. I've finally gotten a
chance to start looking at this again. Making the module changes with
hashes does seem to simplify things. As to performance - besides your
point above about most of the work happening in the scripts, I believe
that this shouldn't be worse than mod_perl 1.x anyway since it is also
based on hash objects. I suppose most people probably avoid PerlRun or
Registry as well if optimization is that important to them that they
would actually care about the performance difference between an array
based object and hash based object.
Also, I followed your suggestion and skipped making accessors for each
member. When I started implementing them it seemed a little excessive
when using a hash based object.
Below is patches to RegistryCooker and RegistryLoader to use hash based
objects. The following is a patched and tested (passes make test)
against the latest CVS. Please let me know what you think.
RegistryCooker.pm:
---------
--- RegistryCooker.pm.cvs 2003-02-25 22:49:44.000000000 -0600
+++ RegistryCooker.pm 2003-02-25 23:23:28.000000000 -0600
@@ -55,17 +55,6 @@
# : D_NONE;
#########################################################################
-# object's array index's access constants
-#
-#########################################################################
-use constant REQ => 0;
-use constant FILENAME => 1;
-use constant URI => 2;
-use constant MTIME => 3;
-use constant PACKAGE => 4;
-use constant CODE => 5;
-
-#########################################################################
# OS specific constants
#
#########################################################################
@@ -100,7 +89,7 @@
sub new {
my($class, $r) = @_;
- my $self = bless [], $class;
+ my $self = bless {}, $class;
$self->init($r);
return $self;
}
@@ -114,9 +103,9 @@
#########################################################################
sub init {
- $_[0]->[REQ] = $_[1];
- $_[0]->[URI] = $_[1]->uri;
- $_[0]->[FILENAME] = $_[1]->filename;
+ $_[0]->{REQ} = $_[1];
+ $_[0]->{URI} = $_[1]->uri;
+ $_[0]->{FILENAME} = $_[1]->filename;
}
#########################################################################
@@ -161,9 +150,9 @@
# handlers shouldn't set $r->status but return it, so we reset the
# status after running it
- my $old_status = $self->[REQ]->status;
+ my $old_status = $self->{REQ}->status;
my $rc = $self->run;
- my $new_status = $self->[REQ]->status($old_status);
+ my $new_status = $self->{REQ}->status($old_status);
return ($rc == Apache::OK && $old_status != $new_status)
? $new_status
: $rc;
@@ -180,8 +169,8 @@
sub run {
my $self = shift;
- my $r = $self->[REQ];
- my $package = $self->[PACKAGE];
+ my $r = $self->{REQ};
+ my $package = $self->{PACKAGE};
$self->set_script_name;
$self->chdir_file;
@@ -227,30 +216,30 @@
sub can_compile {
my $self = shift;
- my $r = $self->[REQ];
+ my $r = $self->{REQ};
unless (-r $r->my_finfo && -s _) {
- $self->log_error("$self->[FILENAME] not found or unable to
stat");
+ $self->log_error("$self->{FILENAME} not found or unable to
stat");
return Apache::NOT_FOUND;
}
return Apache::DECLINED if -d _;
- $self->[MTIME] = -M _;
+ $self->{MTIME} = -M _;
unless (-x _ or IS_WIN32) {
$r->log_error("file permissions deny server execution",
- $self->[FILENAME]);
+ $self->{FILENAME});
return Apache::FORBIDDEN;
}
if (!($r->allow_options & Apache::OPT_EXECCGI)) {
$r->log_error("Options ExecCGI is off in this directory",
- $self->[FILENAME]);
+ $self->{FILENAME});
return Apache::FORBIDDEN;
}
- $self->debug("can compile $self->[FILENAME]") if DEBUG & D_NOISE;
+ $self->debug("can compile $self->{FILENAME}") if DEBUG & D_NOISE;
return Apache::OK;
@@ -291,7 +280,7 @@
# prepend root
$package = $self->namespace_root() . "::$package";
- $self->[PACKAGE] = $package;
+ $self->{PACKAGE} = $package;
return $package;
}
@@ -311,7 +300,7 @@
my $self = shift;
my ($volume, $dirs, $file) =
- File::Spec::Functions::splitpath($self->[FILENAME]);
+ File::Spec::Functions::splitpath($self->{FILENAME});
my @dirs = File::Spec::Functions::splitdir($dirs);
return join '_', grep { defined && length } $volume, @dirs, $file;
}
@@ -320,14 +309,14 @@
sub namespace_from_uri {
my $self = shift;
- my $path_info = $self->[REQ]->path_info;
- my $script_name = $path_info && $self->[URI] =~ /$path_info$/ ?
- substr($self->[URI], 0, length($self->[URI]) - length($path_info)) :
- $self->[URI];
+ my $path_info = $self->{REQ}->path_info;
+ my $script_name = $path_info && $self->{URI} =~ /$path_info$/ ?
+ substr($self->{URI}, 0, length($self->{URI}) - length($path_info)) :
+ $self->{URI};
if ($ModPerl::RegistryCooker::NameWithVirtualHost &&
- $self->[REQ]->server->is_virtual) {
- my $name = $self->[REQ]->get_server_name;
+ $self->{REQ}->server->is_virtual) {
+ my $name = $self->{REQ}->get_server_name;
$script_name = join "", $name, $script_name if $name;
}
@@ -347,7 +336,7 @@
sub convert_script_to_compiled_handler {
my $self = shift;
- $self->debug("Adding package $self->[PACKAGE]") if DEBUG & D_NOISE;
+ $self->debug("Adding package $self->{PACKAGE}") if DEBUG & D_NOISE;
# get the script's source
$self->read_script;
@@ -359,8 +348,8 @@
# relative require/open will work.
$self->chdir_file;
-# undef &{"$self->[PACKAGE]\::handler"}; unless DEBUG & D_NOISE;
#avoid warnings
-# $self->[PACKAGE]->can('undef_functions') &&
$self->[PACKAGE]->undef_functions;
+# undef &{"$self->{PACKAGE}\::handler"}; unless DEBUG & D_NOISE;
#avoid warnings
+# $self->{PACKAGE}->can('undef_functions') &&
$self->{PACKAGE}->undef_functions;
my $line = $self->get_mark_line;
@@ -368,15 +357,15 @@
my $eval = join '',
'package ',
- $self->[PACKAGE], ";",
+ $self->{PACKAGE}, ";",
"sub handler {\n",
$line,
- ${ $self->[CODE] },
+ ${ $self->{CODE} },
"\n}"; # last line comment without newline?
my $rc = $self->compile(\$eval);
return $rc unless $rc == Apache::OK;
- $self->debug(qq{compiled package \"$self->[PACKAGE]\"}) if DEBUG &
D_NOISE;
+ $self->debug(qq{compiled package \"$self->{PACKAGE}\"}) if DEBUG &
D_NOISE;
#$self->chdir_file("$Apache::Server::CWD/");
@@ -421,7 +410,7 @@
sub cache_it {
my $self = shift;
- $self->cache_table->{ $self->[PACKAGE] }{mtime} = $self->[MTIME];
+ $self->cache_table->{ $self->{PACKAGE} }{mtime} = $self->{MTIME};
}
@@ -436,7 +425,7 @@
sub is_cached {
my $self = shift;
- exists $self->cache_table->{ $self->[PACKAGE] }{mtime};
+ exists $self->cache_table->{ $self->{PACKAGE} }{mtime};
}
@@ -456,9 +445,9 @@
# wasn't modified
sub should_compile_if_modified {
my $self = shift;
- $self->[MTIME] ||= -M $self->[REQ]->my_finfo;
+ $self->{MTIME} ||= -M $self->{REQ}->my_finfo;
!($self->is_cached &&
- $self->cache_table->{ $self->[PACKAGE] }{mtime} <=
$self->[MTIME]);
+ $self->cache_table->{ $self->{PACKAGE} }{mtime} <=
$self->{MTIME});
}
# return false if the package is cached already
@@ -482,10 +471,10 @@
$self->debug("flushing namespace") if DEBUG & D_NOISE;
no strict 'refs';
- my $tab = \%{ $self->[PACKAGE] . '::' };
+ my $tab = \%{ $self->{PACKAGE} . '::' };
for (keys %$tab) {
- my $fullname = join '::', $self->[PACKAGE], $_;
+ my $fullname = join '::', $self->{PACKAGE}, $_;
# code/hash/array/scalar might be imported make sure the gv
# does not point elsewhere before undefing each
if (%$fullname) {
@@ -534,8 +523,8 @@
sub read_script {
my $self = shift;
- $self->debug("reading $self->[FILENAME]") if DEBUG & D_NOISE;
- $self->[CODE] = $self->[REQ]->my_slurp_filename;
+ $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
+ $self->{CODE} = $self->{REQ}->my_slurp_filename;
}
#########################################################################
@@ -560,7 +549,7 @@
sub rewrite_shebang {
my $self = shift;
- my($line) = ${ $self->[CODE] } =~ /^(.*)$/m;
+ my($line) = ${ $self->{CODE} } =~ /^(.*)$/m;
my @cmdline = split /\s+/, $line;
return unless @cmdline;
return unless shift(@cmdline) =~ /^\#!/;
@@ -574,7 +563,7 @@
$prepend .= $switches{$_}->();
}
}
- ${ $self->[CODE] } =~ s/^/$prepend/ if $prepend;
+ ${ $self->{CODE} } =~ s/^/$prepend/ if $prepend;
}
#########################################################################
@@ -586,7 +575,7 @@
#########################################################################
sub set_script_name {
- *0 = \(shift->[FILENAME]);
+ *0 = \(shift->{FILENAME});
}
#########################################################################
@@ -602,7 +591,7 @@
sub chdir_file_normal {
my($self, $dir) = @_;
- # $self->[REQ]->chdir_file($dir ? $dir : $self->[FILENAME]);
+ # $self->{REQ}->chdir_file($dir ? $dir : $self->{FILENAME});
}
#########################################################################
@@ -615,19 +604,19 @@
sub get_mark_line {
my $self = shift;
- $ModPerl::Registry::MarkLine ? "\n#line 1 $self->[FILENAME]\n" :
"";
+ $ModPerl::Registry::MarkLine ? "\n#line 1 $self->{FILENAME}\n" :
"";
}
#########################################################################
# func: strip_end_data_segment
# dflt: strip_end_data_segment
-# desc: remove the trailing non-code from $self->[CODE]
+# desc: remove the trailing non-code from $self->{CODE}
# args: $self - registry blessed object
# rtrn: nothing
#########################################################################
sub strip_end_data_segment {
- ${ +shift->[CODE] } =~ s/__(END|DATA)__(.*)//s;
+ ${ +shift->{CODE} } =~ s/__(END|DATA)__(.*)//s;
}
@@ -644,11 +633,11 @@
sub compile {
my($self, $eval) = @_;
- my $r = $self->[REQ];
+ my $r = $self->{REQ};
- $self->debug("compiling $self->[FILENAME]") if DEBUG && D_COMPILE;
+ $self->debug("compiling $self->{FILENAME}") if DEBUG && D_COMPILE;
- ModPerl::Global::special_list_clear(END => $self->[PACKAGE]);
+ ModPerl::Global::special_list_clear(END => $self->{PACKAGE});
ModPerl::Util::untaint($$eval);
{
@@ -707,16 +696,16 @@
sub debug {
my $self = shift;
my $class = ref $self;
- $self->[REQ]->log_error("$$: $class: " . join '', @_);
+ $self->{REQ}->log_error("$$: $class: " . join '', @_);
}
sub log_error {
my($self, $msg) = @_;
my $class = ref $self;
- $self->[REQ]->log_error("$$: $class: $msg");
- $self->[REQ]->notes->set('error-notes' => $msg);
- $@{$self->[URI]} = $msg;
+ $self->{REQ}->log_error("$$: $class: $msg");
+ $self->{REQ}->notes->set('error-notes' => $msg);
+ $@{$self->{URI}} = $msg;
}
#########################################################################
----
RegistryLoader.pm:
-----------
--- RegistryLoader.pm.cvs 2003-02-25 23:19:23.000000000 -0600
+++ RegistryLoader.pm 2003-02-25 23:19:33.000000000 -0600
@@ -104,7 +104,7 @@
# specified by the 'package' attribute, not RegistryLoader
sub namespace_root {
join '::', ModPerl::RegistryCooker::NAMESPACE_ROOT,
- shift->[ModPerl::RegistryCooker::REQ]->{package};
+ shift->{REQ}->{package};
}
# override Apache class methods called by Modperl::Registry*. normally
---
Thanks,
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Wed, 2003-02-12 at 00:23, Stas Bekman wrote:
> Nathan Byrd wrote:
> [...]
> > fields Pragma
> > -------------
> > Advantages:
> > * More straightforward approach
> > * Less code to add to module
> >
> > Disadvantages:
> > * Based on pseudo-hashes - may have problems with reloading, etc
> > * Unknown whether it changes performance (due to "typed lexical"
> > limitation of pragma)
> > * Forces developers to 'use base' and/or 'use fields' in sub-class, can
> > cause non-obvious error otherwise
> >
> > Array based
> > -----------
> > Advantages:
> > * Easy to change code to change base class implementation in the future
> > * Good encapsulation (ability to override methods in future without
> > breaking sub-classes)
> >
> > Disadvantages:
> > * Adds a custom non-standard extending technique
> > * Need to explain method in doco for adding sub-class data (via
> > "public"), vs. pointing the developer to an existing perl document.
> > * Doesn't work with multiple inheritance (probably not a big deal for
> > most RegistryCooker sub-class developers though)
>
> Thanks for the summary Nathan,
>
> Looking at it, I favor your original proposal, as then we have a complete
> control and any bugs are our own fault.
>
> Considering that we go with that approach. Are there any in-core modules that
> can be used for creating/managing accessors? I know there is a whole lot of
> them on CPAN, but we don't want to create extra dependencies.
>
I haven't found any in-core that I know of - but I'll take another look.
> Another approach to simplify your original suggestion, is to move on to hash
> based objects. I doubt it'll be of any significant difference, since most work
> happens in the scripts themselves. I guess a benchmark will show better. But
> if we go with it, subclasses can simply bypass the accessors (we could just
> skip them) and work directly with the hash. Dunno, what's the best way to go.
>
I kinda like this idea. I think we may *still* want to have
accessors/mutators, for the reasons above (encapsulation, etc), but with
a hash based implementation we may be able to just write
accessors/mutators manually for the base instead of using the custom
extension stuff (and sub-classes could do the same). It seems to me
that if a mechanism for encapsulation is provided, and the developer
bypasses it, then they get what they deserve in terms of their module
breaking with future releases (especially if the hash elements are named
with a leading underscore.) The performance could be an issue - let me
think about this a little and do some playing around with the code
tonight.
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Nathan Byrd wrote:
[...]
> fields Pragma
> -------------
> Advantages:
> * More straightforward approach
> * Less code to add to module
>
> Disadvantages:
> * Based on pseudo-hashes - may have problems with reloading, etc
> * Unknown whether it changes performance (due to "typed lexical"
> limitation of pragma)
> * Forces developers to 'use base' and/or 'use fields' in sub-class, can
> cause non-obvious error otherwise
>
> Array based
> -----------
> Advantages:
> * Easy to change code to change base class implementation in the future
> * Good encapsulation (ability to override methods in future without
> breaking sub-classes)
>
> Disadvantages:
> * Adds a custom non-standard extending technique
> * Need to explain method in doco for adding sub-class data (via
> "public"), vs. pointing the developer to an existing perl document.
> * Doesn't work with multiple inheritance (probably not a big deal for
> most RegistryCooker sub-class developers though)
Thanks for the summary Nathan,
Looking at it, I favor your original proposal, as then we have a complete
control and any bugs are our own fault.
Considering that we go with that approach. Are there any in-core modules that
can be used for creating/managing accessors? I know there is a whole lot of
them on CPAN, but we don't want to create extra dependencies.
Another approach to simplify your original suggestion, is to move on to hash
based objects. I doubt it'll be of any significant difference, since most work
happens in the scripts themselves. I guess a benchmark will show better. But
if we go with it, subclasses can simply bypass the accessors (we could just
skip them) and work directly with the hash. Dunno, what's the best way to go.
__________________________________________________________________
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
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Tue, 2003-02-11 at 18:10, Stas Bekman wrote:
> Are there any other folks out there that care about the implementation
> details? That's a pure Perl afterall. It'd be nice to hear more opinions.
>
[...]
> > I was referring to the fact that AFAIK the fields pragma is currently
> > (with 5.61 anyway, haven't checked with 5.8) implemented in terms of
> > pseudo-hashes. Not really important though since we don't have to mess
> > with them directly.
>
> That sucks then. e.g. Apache::Reload doesn't work with pseudo-hashes, but I'm
> not sure if this affects the fields pragma.
>
Yeah, not sure about that. I do know that its implemented with
pseudo-hashes with 5.6.1, if for no other reason than the error that I
was trying to resolve earlier was:
"No such pseudo-hash field "REQ" at " ...
[...]
> > On the other hand, with this extending technique we also gain the
> > flexibility of changing any of those accessors/mutators in the future to
> > do input parameter checking, side effects, etc. To do that we would just
> > make a accessor/mutator by hand. For example:
> > sub FILENAME {
> > my $self = shift;
> > my $val = undef;
> > if(@_) {
> > $val = $_[0];
> > ... code to check value, change $val, return, etc ...
> > $self->[_FILENAME] = $val;
> > }
> > return $self->[_FILENAME];
> > }
> > And rename the current FILENAME accessor/mutator in the call to public.
> > We could also decide later to make some of the members read only this
> > way, deprecate them using warnings, etc.
>
> Sounds good.
>
The problem with trying to do this with the fields pragma is that we
would still need to automatically create accessors/mutators if we want
to encapsulate this data to be able to do the above, and as such would
end up with an equivalent extending technique plus the changes for the
fields pragma, which is probably not what we want.
[...]
> > but its the only way I've found so far to support
> > both mod_perl 1.x Apache::Registry and ModPerl::RegistryCooker from the
> > same module.) Basically, my module does something like the following (in
> > Apache::PAR::Registry):
> >
> > if(eval "Apache::exists_config_define('MODPERL2')") {
> > @ISA = qw(Exporter Apache::PAR::RegistryCooker);
> > require Apache::PAR::RegistryCooker;
> > }
> > else {
> > @ISA = qw(Exporter Apache::PAR::ScriptBase Apache::RegistryNG);
> > require Apache::RegistryNG;
> > require Apache::PAR::ScriptBase;
> > }
>
> Use a BEGIN {} block?
>
Actually, I was incorrect about what the problem was (sorry, too much
coding, not enough sleep :-)
It looks like the reason that this was failing is that the use fields
pragma requires that all sub-classes involved either 'use fields ...' or
'use base ...'. The reason for this is that 'use base' also modifies
the %FIELDS hash in the current package to include the fields from the
parent package (similar to what my original patch does with "import").
So, I got it working by changing the code to be:
if(eval "Apache::exists_config_define('MODPERL2')") {
eval "use base qw(Apache::PAR::RegistryCooker);";
...
Still kinda ugly in my opinion though. Probably not a very likely case
to have to do this however unless a module is trying to work with both
Apache 1.x and 2.x, but it could lead to some strange looking problems
if someone doesn't follow the directions.
We've had a few emails back and forth now, so below is a summarization
of the features/disadvantages of the two approaches as I understand them
from what we have discussed. Please let me know if I miss anything.
I'll try to be as impartial as possible with the following - actually,
after these emails back and forth, I'm not sure if I have a clear
preference myself anymore :-).
fields Pragma
-------------
Advantages:
* More straightforward approach
* Less code to add to module
Disadvantages:
* Based on pseudo-hashes - may have problems with reloading, etc
* Unknown whether it changes performance (due to "typed lexical"
limitation of pragma)
* Forces developers to 'use base' and/or 'use fields' in sub-class, can
cause non-obvious error otherwise
Array based
-----------
Advantages:
* Easy to change code to change base class implementation in the future
* Good encapsulation (ability to override methods in future without
breaking sub-classes)
Disadvantages:
* Adds a custom non-standard extending technique
* Need to explain method in doco for adding sub-class data (via
"public"), vs. pointing the developer to an existing perl document.
* Doesn't work with multiple inheritance (probably not a big deal for
most RegistryCooker sub-class developers though)
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Are there any other folks out there that care about the implementation
details? That's a pure Perl afterall. It'd be nice to hear more opinions.
Nathan Byrd wrote:
[...]
>>>2) Anything that looks like pseudo-hashes scares me. :-) Pseudo-hashes
>>>seem to be in flux, and as a result I went with the more "old fashioned"
>>>approach.
>>
>>Yes, pseudo-hashes are out of question.
>>
>
>
> I was referring to the fact that AFAIK the fields pragma is currently
> (with 5.61 anyway, haven't checked with 5.8) implemented in terms of
> pseudo-hashes. Not really important though since we don't have to mess
> with them directly.
That sucks then. e.g. Apache::Reload doesn't work with pseudo-hashes, but I'm
not sure if this affects the fields pragma.
>>>I can also see it either way though:
>>>1) It would be less code to implement this change as a fields pragma.
>>>2) The fields pragma is promised to work as advertised even when the
>>>current pseudo-hash implementation goes away (as per the perlref doco.)
>>>
>>>I think overall I currently feel more comfortable with the array based
>>>patch mostly because even though its more lines, it seems to remain more
>>>familiarity to the original code. Also I felt that any performance or
>>>integration testing that has already been done with Registry scripts may
>>>be slightly invalidated with a change to the implementation structure of
>>>the module.
>>
>>But this introduces a custom non-standard extending technique, which should
>>rather stay away from if we can.
>>
>
>
> I definitely agree with this point. Overall, using a fields pragma may
> be cleaner in that regard.
>
> On the other hand, with this extending technique we also gain the
> flexibility of changing any of those accessors/mutators in the future to
> do input parameter checking, side effects, etc. To do that we would just
> make a accessor/mutator by hand. For example:
> sub FILENAME {
> my $self = shift;
> my $val = undef;
> if(@_) {
> $val = $_[0];
> ... code to check value, change $val, return, etc ...
> $self->[_FILENAME] = $val;
> }
> return $self->[_FILENAME];
> }
> And rename the current FILENAME accessor/mutator in the call to public.
> We could also decide later to make some of the members read only this
> way, deprecate them using warnings, etc.
Sounds good.
>>The test suite is covering enough to safely verify any refactorings, so I'm
>>not worried about completely changing the internal object implementation. And
>>no performance benchmarking was done so far.
>>
>>
>>>While I was researching your question I mostly coded a solution to make
>>>this change with the fields pragma, just haven't tested it yet. If you
>>>want, I can also post that version to compare the current proposed patch
>>>with.
>>
>>It'd be nice to keep the extension mechanism as simple as possible, as long as
>>we don't lose in performance. The fields manpage says that the fields are
>>compiled, so perhaps using those will give us the same performance (or equal
>>enough) compared with array-based objects. Is there some document that
>>compares these two? I'm sure Damian's book talks about them all, but I'm not
>>sure if there is a performance comparison there.
>>
>
>
> I'm wrestling with this right now - the documentation for fields says:
>
> "The effect of all this is that you can have objects with named fields
> which are as compact as arrays and as fast to access. This only works as
> long as the objects are accessed through properly typed lexical
> variables, though. If the variables are not typed, access is only
> checked at run time, so your program runs slower because it has to do
> both a hash access and an array access."
>
> Since this is compile time does this mean it has to be done throughout?
> If they need to be typed throughout we could lose some performance as I
> assume mod_perl can't type the lexical when calling handler as a method,
> unless the following (in ModPerl::RegistryCooker) would suffice to
> satisfy this:
> my ModPerl::RegistryCooker $class = (@_ >= 2) ? shift : __PACKAGE__;
>
> I've tested re-implementing the changes using the fields pragma, and it
> seems to run fine for registry scripts. I've run into another problem
> while testing using a fields pragma implementation however - since the
> class fields are setup at compile time, it looks like its not working
> with my Apache::PAR::Registry module which modifies @ISA when called (I
> know, bad idea anyway,
RegistryLoader modifies @INC at run time, so the same module can load scripts
for different registry sub-classes. It simply subclasses itself from the
chosen module dynamically at run-time.
> but its the only way I've found so far to support
> both mod_perl 1.x Apache::Registry and ModPerl::RegistryCooker from the
> same module.) Basically, my module does something like the following (in
> Apache::PAR::Registry):
>
> if(eval "Apache::exists_config_define('MODPERL2')") {
> @ISA = qw(Exporter Apache::PAR::RegistryCooker);
> require Apache::PAR::RegistryCooker;
> }
> else {
> @ISA = qw(Exporter Apache::PAR::ScriptBase Apache::RegistryNG);
> require Apache::RegistryNG;
> require Apache::PAR::ScriptBase;
> }
Use a BEGIN {} block?
__________________________________________________________________
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
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Mon, 2003-02-10 at 22:37, Stas Bekman wrote:
> [...]
>
> >>The patch needs some minor tweaking to conform with our coding style, but
> >>overall it looks good. Though before we move on with this, won't it be simpler
> >>to use the 'fields' pragma?
> >
> >
> > Sorry about that - I'll take another look at the mod_perl coding
> > guidelines before submitting another patch.
>
> That's not a problem. let's first figure out what's the best approach to take,
> before wasting time on fixing the style.
Agreed.
>
> > Using fields would definitely be a good way of going about it. I didn't
> > choose that approach in the code I submitted because:
> > 1) I felt that implementing RegistryCooker as an array based object was
> > a good design philosophy, and my changes (hopefully) shouldn't have much
> > of an impact on the current working of the module unless its used in a
> > subclass (and we have a win in that situation.)
>
> A good design philosophy is when the parent class can change its internal
> object implementation without affecting its sub-classes ;) I simply didn't
> think of sub-classes wanting to extend the object itself, but only to override
> some methods.
>
I think both solutions probably meet this goal - with my original patch
it can be done by changing a couple of subs (public and make_accessor).
With the fields pragma, we could change the underlying implementation as
well (although without changing any subclasses we could really only
change to a hash implementation, or something else that emulated one.)
> > 2) Anything that looks like pseudo-hashes scares me. :-) Pseudo-hashes
> > seem to be in flux, and as a result I went with the more "old fashioned"
> > approach.
>
> Yes, pseudo-hashes are out of question.
>
I was referring to the fact that AFAIK the fields pragma is currently
(with 5.61 anyway, haven't checked with 5.8) implemented in terms of
pseudo-hashes. Not really important though since we don't have to mess
with them directly.
> > I can also see it either way though:
> > 1) It would be less code to implement this change as a fields pragma.
> > 2) The fields pragma is promised to work as advertised even when the
> > current pseudo-hash implementation goes away (as per the perlref doco.)
> >
> > I think overall I currently feel more comfortable with the array based
> > patch mostly because even though its more lines, it seems to remain more
> > familiarity to the original code. Also I felt that any performance or
> > integration testing that has already been done with Registry scripts may
> > be slightly invalidated with a change to the implementation structure of
> > the module.
>
> But this introduces a custom non-standard extending technique, which should
> rather stay away from if we can.
>
I definitely agree with this point. Overall, using a fields pragma may
be cleaner in that regard.
On the other hand, with this extending technique we also gain the
flexibility of changing any of those accessors/mutators in the future to
do input parameter checking, side effects, etc. To do that we would just
make a accessor/mutator by hand. For example:
sub FILENAME {
my $self = shift;
my $val = undef;
if(@_) {
$val = $_[0];
... code to check value, change $val, return, etc ...
$self->[_FILENAME] = $val;
}
return $self->[_FILENAME];
}
And rename the current FILENAME accessor/mutator in the call to public.
We could also decide later to make some of the members read only this
way, deprecate them using warnings, etc.
> The test suite is covering enough to safely verify any refactorings, so I'm
> not worried about completely changing the internal object implementation. And
> no performance benchmarking was done so far.
>
> > While I was researching your question I mostly coded a solution to make
> > this change with the fields pragma, just haven't tested it yet. If you
> > want, I can also post that version to compare the current proposed patch
> > with.
>
> It'd be nice to keep the extension mechanism as simple as possible, as long as
> we don't lose in performance. The fields manpage says that the fields are
> compiled, so perhaps using those will give us the same performance (or equal
> enough) compared with array-based objects. Is there some document that
> compares these two? I'm sure Damian's book talks about them all, but I'm not
> sure if there is a performance comparison there.
>
I'm wrestling with this right now - the documentation for fields says:
"The effect of all this is that you can have objects with named fields
which are as compact as arrays and as fast to access. This only works as
long as the objects are accessed through properly typed lexical
variables, though. If the variables are not typed, access is only
checked at run time, so your program runs slower because it has to do
both a hash access and an array access."
Since this is compile time does this mean it has to be done throughout?
If they need to be typed throughout we could lose some performance as I
assume mod_perl can't type the lexical when calling handler as a method,
unless the following (in ModPerl::RegistryCooker) would suffice to
satisfy this:
my ModPerl::RegistryCooker $class = (@_ >= 2) ? shift : __PACKAGE__;
I've tested re-implementing the changes using the fields pragma, and it
seems to run fine for registry scripts. I've run into another problem
while testing using a fields pragma implementation however - since the
class fields are setup at compile time, it looks like its not working
with my Apache::PAR::Registry module which modifies @ISA when called (I
know, bad idea anyway, but its the only way I've found so far to support
both mod_perl 1.x Apache::Registry and ModPerl::RegistryCooker from the
same module.) Basically, my module does something like the following (in
Apache::PAR::Registry):
if(eval "Apache::exists_config_define('MODPERL2')") {
@ISA = qw(Exporter Apache::PAR::RegistryCooker);
require Apache::PAR::RegistryCooker;
}
else {
@ISA = qw(Exporter Apache::PAR::ScriptBase Apache::RegistryNG);
require Apache::RegistryNG;
require Apache::PAR::ScriptBase;
}
The original patch works with this because all of the real work is done
in Apache::PAR::RegistryCooker, which is a sub-class of
ModPerl::RegistryCooker, and so creates the accessors correctly.
Unfortunately, I'm running out of time tonight to make a simpler test
case, I'll have to pick this up tomorrow.
Thanks,
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
[...]
>>The patch needs some minor tweaking to conform with our coding style, but
>>overall it looks good. Though before we move on with this, won't it be simpler
>>to use the 'fields' pragma?
>
>
> Sorry about that - I'll take another look at the mod_perl coding
> guidelines before submitting another patch.
That's not a problem. let's first figure out what's the best approach to take,
before wasting time on fixing the style.
> Using fields would definitely be a good way of going about it. I didn't
> choose that approach in the code I submitted because:
> 1) I felt that implementing RegistryCooker as an array based object was
> a good design philosophy, and my changes (hopefully) shouldn't have much
> of an impact on the current working of the module unless its used in a
> subclass (and we have a win in that situation.)
A good design philosophy is when the parent class can change its internal
object implementation without affecting its sub-classes ;) I simply didn't
think of sub-classes wanting to extend the object itself, but only to override
some methods.
> 2) Anything that looks like pseudo-hashes scares me. :-) Pseudo-hashes
> seem to be in flux, and as a result I went with the more "old fashioned"
> approach.
Yes, pseudo-hashes are out of question.
> I can also see it either way though:
> 1) It would be less code to implement this change as a fields pragma.
> 2) The fields pragma is promised to work as advertised even when the
> current pseudo-hash implementation goes away (as per the perlref doco.)
>
> I think overall I currently feel more comfortable with the array based
> patch mostly because even though its more lines, it seems to remain more
> familiarity to the original code. Also I felt that any performance or
> integration testing that has already been done with Registry scripts may
> be slightly invalidated with a change to the implementation structure of
> the module.
But this introduces a custom non-standard extending technique, which should
rather stay away from if we can.
The test suite is covering enough to safely verify any refactorings, so I'm
not worried about completely changing the internal object implementation. And
no performance benchmarking was done so far.
> While I was researching your question I mostly coded a solution to make
> this change with the fields pragma, just haven't tested it yet. If you
> want, I can also post that version to compare the current proposed patch
> with.
It'd be nice to keep the extension mechanism as simple as possible, as long as
we don't lose in performance. The fields manpage says that the fields are
compiled, so perhaps using those will give us the same performance (or equal
enough) compared with array-based objects. Is there some document that
compares these two? I'm sure Damian's book talks about them all, but I'm not
sure if there is a performance comparison there.
__________________________________________________________________
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
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
On Mon, 2003-02-10 at 16:42, Stas Bekman wrote:
> Nathan Byrd wrote:
> > Hi all,
> >
> > Below is an initial version of a patch against the latest CVS version of
> > RegistryCooker.pm (and RegistryLoader.pm) to support better subclassing,
> > including the ability to access module data from RegistryCooker in a
> > clean way and to add private data to subclasses, without relying on the
> > underlying array implementation (below my patch is the original message
> > written to the mod_perl list and Stas Bekman's response to provide the
> > context for this.)
> >
> > The biggest part of this patch is from a change to the constants in
> > RegistryCooker to be _REQ instead of REQ, etc, because I couldn't have
> > two subs with the same name, and these constants are really internal
> > data anyway, especially if you want to be able to not rely on
> > RegistryCooker being implemented as an array. Let me know if this is a
> > problem, I can also do it the other way around (another possibility
> > would be to change them instead to something like REQ_IDX or something,
> > but its kinda wordy).
> >
> > To use this in a subclass, I call "public" with this names of the
> > variables I want to use for my subclass, then use them like normal: eg:
> >
> > public qw(
> > PAR_MEMBER
> > ...
> > );
> > ...
> > $self->PAR_MEMBER(<value>); # Set a value
> > ...
> > $something = $self->PAR_MEMBER; #Get a value
> >
> > We can also access the base class data in the same fashion:
> >
> > my $r = $self->REQ;
> >
> >
> > I left the original constant usage inside RegistryCooker for performance
> > (no need to do an extra sub call inside RegistryCooker itself).
> >
> > Please take a look at this and let me know what you think - I've tested
> > it under my configuration with the latest CVS and a now much nicer
> > looking version :-) of my Apache::PAR module as well as some simple
> > Registry scripts. If this patch is acceptable I'll also send another
> > patch to add the appropriate tests to the test suite and add a section
> > about subclassing RegistryCooker to the porting guidelines and
> > RegistryCooker docs.
>
> Good work, Nathan!
>
> The patch needs some minor tweaking to conform with our coding style, but
> overall it looks good. Though before we move on with this, won't it be simpler
> to use the 'fields' pragma?
>
Sorry about that - I'll take another look at the mod_perl coding
guidelines before submitting another patch.
Using fields would definitely be a good way of going about it. I didn't
choose that approach in the code I submitted because:
1) I felt that implementing RegistryCooker as an array based object was
a good design philosophy, and my changes (hopefully) shouldn't have much
of an impact on the current working of the module unless its used in a
subclass (and we have a win in that situation.)
2) Anything that looks like pseudo-hashes scares me. :-) Pseudo-hashes
seem to be in flux, and as a result I went with the more "old fashioned"
approach.
I can also see it either way though:
1) It would be less code to implement this change as a fields pragma.
2) The fields pragma is promised to work as advertised even when the
current pseudo-hash implementation goes away (as per the perlref doco.)
I think overall I currently feel more comfortable with the array based
patch mostly because even though its more lines, it seems to remain more
familiarity to the original code. Also I felt that any performance or
integration testing that has already been done with Registry scripts may
be slightly invalidated with a change to the implementation structure of
the module.
While I was researching your question I mostly coded a solution to make
this change with the fields pragma, just haven't tested it yet. If you
want, I can also post that version to compare the current proposed patch
with.
Thanks,
--
Nathan Byrd <na...@byrd.net>
>
> __________________________________________________________________
> 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
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Proposed changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Nathan Byrd wrote:
> Hi all,
>
> Below is an initial version of a patch against the latest CVS version of
> RegistryCooker.pm (and RegistryLoader.pm) to support better subclassing,
> including the ability to access module data from RegistryCooker in a
> clean way and to add private data to subclasses, without relying on the
> underlying array implementation (below my patch is the original message
> written to the mod_perl list and Stas Bekman's response to provide the
> context for this.)
>
> The biggest part of this patch is from a change to the constants in
> RegistryCooker to be _REQ instead of REQ, etc, because I couldn't have
> two subs with the same name, and these constants are really internal
> data anyway, especially if you want to be able to not rely on
> RegistryCooker being implemented as an array. Let me know if this is a
> problem, I can also do it the other way around (another possibility
> would be to change them instead to something like REQ_IDX or something,
> but its kinda wordy).
>
> To use this in a subclass, I call "public" with this names of the
> variables I want to use for my subclass, then use them like normal: eg:
>
> public qw(
> PAR_MEMBER
> ...
> );
> ...
> $self->PAR_MEMBER(<value>); # Set a value
> ...
> $something = $self->PAR_MEMBER; #Get a value
>
> We can also access the base class data in the same fashion:
>
> my $r = $self->REQ;
>
>
> I left the original constant usage inside RegistryCooker for performance
> (no need to do an extra sub call inside RegistryCooker itself).
>
> Please take a look at this and let me know what you think - I've tested
> it under my configuration with the latest CVS and a now much nicer
> looking version :-) of my Apache::PAR module as well as some simple
> Registry scripts. If this patch is acceptable I'll also send another
> patch to add the appropriate tests to the test suite and add a section
> about subclassing RegistryCooker to the porting guidelines and
> RegistryCooker docs.
Good work, Nathan!
The patch needs some minor tweaking to conform with our coding style, but
overall it looks good. Though before we move on with this, won't it be simpler
to use the 'fields' pragma?
__________________________________________________________________
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
[patch] Proposed changes to RegistryCooker for subclassing
Posted by Nathan Byrd <na...@byrd.net>.
Hi all,
Below is an initial version of a patch against the latest CVS version of
RegistryCooker.pm (and RegistryLoader.pm) to support better subclassing,
including the ability to access module data from RegistryCooker in a
clean way and to add private data to subclasses, without relying on the
underlying array implementation (below my patch is the original message
written to the mod_perl list and Stas Bekman's response to provide the
context for this.)
The biggest part of this patch is from a change to the constants in
RegistryCooker to be _REQ instead of REQ, etc, because I couldn't have
two subs with the same name, and these constants are really internal
data anyway, especially if you want to be able to not rely on
RegistryCooker being implemented as an array. Let me know if this is a
problem, I can also do it the other way around (another possibility
would be to change them instead to something like REQ_IDX or something,
but its kinda wordy).
To use this in a subclass, I call "public" with this names of the
variables I want to use for my subclass, then use them like normal: eg:
public qw(
PAR_MEMBER
...
);
...
$self->PAR_MEMBER(<value>); # Set a value
...
$something = $self->PAR_MEMBER; #Get a value
We can also access the base class data in the same fashion:
my $r = $self->REQ;
I left the original constant usage inside RegistryCooker for performance
(no need to do an extra sub call inside RegistryCooker itself).
Please take a look at this and let me know what you think - I've tested
it under my configuration with the latest CVS and a now much nicer
looking version :-) of my Apache::PAR module as well as some simple
Registry scripts. If this patch is acceptable I'll also send another
patch to add the appropriate tests to the test suite and add a section
about subclassing RegistryCooker to the porting guidelines and
RegistryCooker docs.
Thanks,
--
Nathan Byrd <na...@byrd.net>
~~~~~~~
--- RegistryCooker.pm Sun Feb 9 22:53:45 2003
+++ RegistryCooker.pm.new Sun Feb 9 22:56:10 2003
@@ -35,6 +35,94 @@
}
#########################################################################
+# code for automatic accessors/mutators
+#
+#########################################################################
+
+my $_self_offset = 0;
+
+#########################################################################
+# func: import
+# dflt: import
+# desc: Overrides default import to handle accessor creation
+# args: nothing
+# rtrn: nothing
+#########################################################################
+
+sub import
+{
+ no strict 'refs'; # turn off just for this sub
+ my $caller = (caller)[0];
+ *{"${caller}::public"} = \&public;
+ *{"${caller}::import"} = \&import;
+}
+
+
+#########################################################################
+# func: get_attr_offset
+# dflt: get_attr_offset
+# desc: returns the offset for a given package in the ISA tree
+# args: nothing
+# rtrn: number of attributes defined for this package
+# note: this function is recursive
+#########################################################################
+
+sub get_attr_offset
+{
+ no strict 'refs'; # turn off just for this sub
+ my $class = shift;
+ my $offset = 0;
+ foreach my $parent (@{"${class}::ISA"}) {
+ $offset += ${parent}->get_attr_offset()
if(${parent}->can('get_attr_offset'));
+ }
+ return $offset + $_self_offset;
+}
+
+#########################################################################
+# func: public
+# dflt: public
+# desc: creates accessor functions
+# args: list of accessors to create
+# rtrn: nothing
+# note: prototype defined so that public qw( ... ); can be used
+#########################################################################
+
+sub public(@)
+{
+ no strict 'refs'; # turn off just for this sub
+ my $class = (caller)[0];
+ my $parent_offset = ${class}->get_attr_offset;
+ for my $attr_name (@_)
+ {
+ if(!${class}->can($attr_name)) {
+ *{"${class}::${attr_name}"} = ${class}->make_accessor(
+ $_self_offset + $parent_offset);
+ $_self_offset++;
+ }
+ else { warn("$attr_name already defined"); }
+ }
+}
+
+#########################################################################
+# func: make_accessor
+# dflt: make_accessor
+# desc: does the work of creating an accessor
+# args: index to use for the accessor
+# rtrn: subroutine to be installed as accessor
+#########################################################################
+
+sub make_accessor
+{
+ my $index = $_[1];
+ return sub {
+ my $self = shift;
+ $self->[$index] = $_[0] if @_;
+ return $self->[$index];
+ }
+}
+
+
+#########################################################################
# debug constants
#
#########################################################################
@@ -58,14 +146,27 @@
# object's array index's access constants
#
#########################################################################
-use constant REQ => 0;
-use constant FILENAME => 1;
-use constant URI => 2;
-use constant MTIME => 3;
-use constant PACKAGE => 4;
-use constant CODE => 5;
+use constant _REQ => 0;
+use constant _FILENAME => 1;
+use constant _URI => 2;
+use constant _MTIME => 3;
+use constant _PACKAGE => 4;
+use constant _CODE => 5;
#########################################################################
+# base class accessors/mutators for access to data
+# note: should be in same order as above
+#
+#########################################################################
+public qw(
+ REQ
+ FILENAME
+ URI
+ MTIME
+ PACKAGE
+ CODE
+);
+#########################################################################
# OS specific constants
#
#########################################################################
@@ -108,15 +209,15 @@
#########################################################################
# func: init
# dflt: init
-# desc: initializes the data object's fields: REQ FILENAME URI
+# desc: initializes the data object's fields: _REQ _FILENAME _URI
# args: $r - Apache::Request object
# rtrn: nothing
#########################################################################
sub init {
- $_[0]->[REQ] = $_[1];
- $_[0]->[URI] = $_[1]->uri;
- $_[0]->[FILENAME] = $_[1]->filename;
+ $_[0]->[_REQ] = $_[1];
+ $_[0]->[_URI] = $_[1]->uri;
+ $_[0]->[_FILENAME] = $_[1]->filename;
}
#########################################################################
@@ -161,9 +262,9 @@
# handlers shouldn't set $r->status but return it, so we reset the
# status after running it
- my $old_status = $self->[REQ]->status;
+ my $old_status = $self->[_REQ]->status;
my $rc = $self->run;
- my $new_status = $self->[REQ]->status($old_status);
+ my $new_status = $self->[_REQ]->status($old_status);
return ($rc == Apache::OK && $old_status != $new_status)
? $new_status
: $rc;
@@ -180,8 +281,8 @@
sub run {
my $self = shift;
- my $r = $self->[REQ];
- my $package = $self->[PACKAGE];
+ my $r = $self->[_REQ];
+ my $package = $self->[_PACKAGE];
$self->set_script_name;
$self->chdir_file;
@@ -222,35 +323,35 @@
# desc: checks whether the script is allowed and can be compiled
# args: $self - registry blessed object
# rtrn: $rc - return status to forward
-# efct: initializes the data object's fields: MTIME
+# efct: initializes the data object's fields: _MTIME
#########################################################################
sub can_compile {
my $self = shift;
- my $r = $self->[REQ];
+ my $r = $self->[_REQ];
unless (-r $r->my_finfo && -s _) {
- $self->log_error("$self->[FILENAME] not found or unable to
stat");
+ $self->log_error("$self->[_FILENAME] not found or unable to
stat");
return Apache::NOT_FOUND;
}
return Apache::DECLINED if -d _;
- $self->[MTIME] = -M _;
+ $self->[_MTIME] = -M _;
unless (-x _ or IS_WIN32) {
$r->log_error("file permissions deny server execution",
- $self->[FILENAME]);
+ $self->[_FILENAME]);
return Apache::FORBIDDEN;
}
if (!($r->allow_options & Apache::OPT_EXECCGI)) {
$r->log_error("Options ExecCGI is off in this directory",
- $self->[FILENAME]);
+ $self->[_FILENAME]);
return Apache::FORBIDDEN;
}
- $self->debug("can compile $self->[FILENAME]") if DEBUG & D_NOISE;
+ $self->debug("can compile $self->[_FILENAME]") if DEBUG & D_NOISE;
return Apache::OK;
@@ -274,7 +375,7 @@
# desc: prepares the namespace
# args: $self - registry blessed object
# rtrn: the namespace
-# efct: initializes the field: PACKAGE
+# efct: initializes the field: _PACKAGE
#########################################################################
sub make_namespace {
@@ -291,7 +392,7 @@
# prepend root
$package = $self->namespace_root() . "::$package";
- $self->[PACKAGE] = $package;
+ $self->[_PACKAGE] = $package;
return $package;
}
@@ -311,7 +412,7 @@
my $self = shift;
my ($volume, $dirs, $file) =
- File::Spec::Functions::splitpath($self->[FILENAME]);
+ File::Spec::Functions::splitpath($self->[_FILENAME]);
my @dirs = File::Spec::Functions::splitdir($dirs);
return join '_', grep { defined && length } $volume, @dirs, $file;
}
@@ -320,14 +421,14 @@
sub namespace_from_uri {
my $self = shift;
- my $path_info = $self->[REQ]->path_info;
- my $script_name = $path_info && $self->[URI] =~ /$path_info$/ ?
- substr($self->[URI], 0, length($self->[URI]) - length($path_info)) :
- $self->[URI];
+ my $path_info = $self->[_REQ]->path_info;
+ my $script_name = $path_info && $self->[_URI] =~ /$path_info$/ ?
+ substr($self->[_URI], 0, length($self->[_URI]) - length($path_info)) :
+ $self->[_URI];
if ($ModPerl::RegistryCooker::NameWithVirtualHost &&
- $self->[REQ]->server->is_virtual) {
- my $name = $self->[REQ]->get_server_name;
+ $self->[_REQ]->server->is_virtual) {
+ my $name = $self->[_REQ]->get_server_name;
$script_name = join "", $name, $script_name if $name;
}
@@ -347,7 +448,7 @@
sub convert_script_to_compiled_handler {
my $self = shift;
- $self->debug("Adding package $self->[PACKAGE]") if DEBUG & D_NOISE;
+ $self->debug("Adding package $self->[_PACKAGE]") if DEBUG &
D_NOISE;
# get the script's source
$self->read_script;
@@ -359,8 +460,8 @@
# relative require/open will work.
$self->chdir_file;
-# undef &{"$self->[PACKAGE]\::handler"}; unless DEBUG & D_NOISE;
#avoid warnings
-# $self->[PACKAGE]->can('undef_functions') &&
$self->[PACKAGE]->undef_functions;
+# undef &{"$self->[_PACKAGE]\::handler"}; unless DEBUG & D_NOISE;
#avoid warnings
+# $self->[_PACKAGE]->can('undef_functions') &&
$self->[_PACKAGE]->undef_functions;
my $line = $self->get_mark_line;
@@ -368,15 +469,15 @@
my $eval = join '',
'package ',
- $self->[PACKAGE], ";",
+ $self->[_PACKAGE], ";",
"sub handler {\n",
$line,
- ${ $self->[CODE] },
+ ${ $self->[_CODE] },
"\n}"; # last line comment without newline?
my $rc = $self->compile(\$eval);
return $rc unless $rc == Apache::OK;
- $self->debug(qq{compiled package \"$self->[PACKAGE]\"}) if DEBUG &
D_NOISE;
+ $self->debug(qq{compiled package \"$self->[_PACKAGE]\"}) if DEBUG &
D_NOISE;
#$self->chdir_file("$Apache::Server::CWD/");
@@ -421,7 +522,7 @@
sub cache_it {
my $self = shift;
- $self->cache_table->{ $self->[PACKAGE] }{mtime} = $self->[MTIME];
+ $self->cache_table->{ $self->[_PACKAGE] }{mtime} = $self->[_MTIME];
}
@@ -436,7 +537,7 @@
sub is_cached {
my $self = shift;
- exists $self->cache_table->{ $self->[PACKAGE] }{mtime};
+ exists $self->cache_table->{ $self->[_PACKAGE] }{mtime};
}
@@ -456,9 +557,9 @@
# wasn't modified
sub should_compile_if_modified {
my $self = shift;
- $self->[MTIME] ||= -M $self->[REQ]->my_finfo;
+ $self->[_MTIME] ||= -M $self->[_REQ]->my_finfo;
!($self->is_cached &&
- $self->cache_table->{ $self->[PACKAGE] }{mtime} <=
$self->[MTIME]);
+ $self->cache_table->{ $self->[_PACKAGE] }{mtime} <=
$self->[_MTIME]);
}
# return false if the package is cached already
@@ -482,10 +583,10 @@
$self->debug("flushing namespace") if DEBUG & D_NOISE;
no strict 'refs';
- my $tab = \%{ $self->[PACKAGE] . '::' };
+ my $tab = \%{ $self->[_PACKAGE] . '::' };
for (keys %$tab) {
- my $fullname = join '::', $self->[PACKAGE], $_;
+ my $fullname = join '::', $self->[_PACKAGE], $_;
# code/hash/array/scalar might be imported make sure the gv
# does not point elsewhere before undefing each
if (%$fullname) {
@@ -527,15 +628,15 @@
# desc: reads the script in
# args: $self - registry blessed object
# rtrn: nothing
-# efct: initializes the CODE field with the source script
+# efct: initializes the _CODE field with the source script
#########################################################################
# reads the contents of the file
sub read_script {
my $self = shift;
- $self->debug("reading $self->[FILENAME]") if DEBUG & D_NOISE;
- $self->[CODE] = $self->[REQ]->my_slurp_filename;
+ $self->debug("reading $self->[_FILENAME]") if DEBUG & D_NOISE;
+ $self->[_CODE] = $self->[_REQ]->my_slurp_filename;
}
#########################################################################
@@ -545,7 +646,7 @@
# (defined in %switches) into a perl code.
# args: $self - registry blessed object
# rtrn: nothing
-# efct: the CODE field gets adjusted
+# efct: the _CODE field gets adjusted
#########################################################################
my %switches = (
@@ -560,7 +661,7 @@
sub rewrite_shebang {
my $self = shift;
- my($line) = ${ $self->[CODE] } =~ /^(.*)$/m;
+ my($line) = ${ $self->[_CODE] } =~ /^(.*)$/m;
my @cmdline = split /\s+/, $line;
return unless @cmdline;
return unless shift(@cmdline) =~ /^\#!/;
@@ -574,7 +675,7 @@
$prepend .= $switches{$_}->();
}
}
- ${ $self->[CODE] } =~ s/^/$prepend/ if $prepend;
+ ${ $self->[_CODE] } =~ s/^/$prepend/ if $prepend;
}
#########################################################################
@@ -586,7 +687,7 @@
#########################################################################
sub set_script_name {
- *0 = \(shift->[FILENAME]);
+ *0 = \(shift->[_FILENAME]);
}
#########################################################################
@@ -602,7 +703,7 @@
sub chdir_file_normal {
my($self, $dir) = @_;
- # $self->[REQ]->chdir_file($dir ? $dir : $self->[FILENAME]);
+ # $self->[_REQ]->chdir_file($dir ? $dir : $self->[_FILENAME]);
}
#########################################################################
@@ -615,19 +716,19 @@
sub get_mark_line {
my $self = shift;
- $ModPerl::Registry::MarkLine ? "\n#line 1 $self->[FILENAME]\n" :
"";
+ $ModPerl::Registry::MarkLine ? "\n#line 1 $self->[_FILENAME]\n" :
"";
}
#########################################################################
# func: strip_end_data_segment
# dflt: strip_end_data_segment
-# desc: remove the trailing non-code from $self->[CODE]
+# desc: remove the trailing non-code from $self->[_CODE]
# args: $self - registry blessed object
# rtrn: nothing
#########################################################################
sub strip_end_data_segment {
- ${ +shift->[CODE] } =~ s/__(END|DATA)__(.*)//s;
+ ${ +shift->[_CODE] } =~ s/__(END|DATA)__(.*)//s;
}
@@ -644,11 +745,11 @@
sub compile {
my($self, $eval) = @_;
- my $r = $self->[REQ];
+ my $r = $self->[_REQ];
- $self->debug("compiling $self->[FILENAME]") if DEBUG && D_COMPILE;
+ $self->debug("compiling $self->[_FILENAME]") if DEBUG && D_COMPILE;
- ModPerl::Global::special_list_clear(END => $self->[PACKAGE]);
+ ModPerl::Global::special_list_clear(END => $self->[_PACKAGE]);
ModPerl::Util::untaint($$eval);
{
@@ -707,16 +808,16 @@
sub debug {
my $self = shift;
my $class = ref $self;
- $self->[REQ]->log_error("$$: $class: " . join '', @_);
+ $self->[_REQ]->log_error("$$: $class: " . join '', @_);
}
sub log_error {
my($self, $msg) = @_;
my $class = ref $self;
- $self->[REQ]->log_error("$$: $class: $msg");
- $self->[REQ]->notes->set('error-notes' => $msg);
- $@{$self->[URI]} = $msg;
+ $self->[_REQ]->log_error("$$: $class: $msg");
+ $self->[_REQ]->notes->set('error-notes' => $msg);
+ $@{$self->[_URI]} = $msg;
}
#########################################################################
~~~~~~~~~~~~~~~~~
RegistryLoader.pm (small, but untested)
~~~~~~~~~~~~~~~~~
--- RegistryLoader.pm Sun Feb 9 23:33:17 2003
+++ RegistryLoader.pm.new Sun Feb 9 23:33:27 2003
@@ -104,7 +104,7 @@
# specified by the 'package' attribute, not RegistryLoader
sub namespace_root {
join '::', ModPerl::RegistryCooker::NAMESPACE_ROOT,
- shift->[ModPerl::RegistryCooker::REQ]->{package};
+ shift->[ModPerl::RegistryCooker::_REQ]->{package};
}
# override Apache class methods called by Modperl::Registry*. normally
On Mon, 2003-02-03 at 00:05, Stas Bekman wrote:
> Nathan Byrd wrote:
> > All,
> >
> > To begin with, should proposed mod_perl patches go to
> > dev@perl.apache.org? The documentation seemed a little unclear in this
> > case (I decided to play it safe since I didn't run across any messages
> > on the dev list from outside developers.)
>
> dev@perl.apache.org would be the right place. Also help with the doc would be
> *very* appreciated, I've started to write the initial doc, but it's a far away
> from being useful.
>
> > When I was converting Apache::PAR to work with mod_perl 2.x, one problem
> > I had was with the way in which ModPerl::RegistryCooker stores member
> > data. Any subclass of ModPerl::RegistryCooker (in my case, for
> > Apache::PAR::RegistryCooker) that need to store their own module data
> > have a problem - they need to pick an array element to store their data
> > in. Because of the way in which ModPerl::RegistryCooker works
> > currently, that means hardcoding an array index (because not all array
> > members are created in new or init). Thus, in
> > Apache::PAR::RegistryCooker I have a line similar to the following:
> >
> > use constant PARDATA => 8;
> >
> > This is not optimal, especially since this has already changed in the
> > CVS version of RegistryCooker since I started working on it - luckily,
> > to less members, not more :-)
> >
> > I propose a change to RegistryCooker.pm so that member data is always
> > defined in the init sub, so that I could change the above line to
> > something more like:
> >
> > use constant PARDATA => -1;
> >
> > and in my handler, push a new element on after new has been called.
> > This would keep future changes to the RegistryCooker script from
> > potentially breaking other modules which must store their own data as
> > well.
> >
> > Below is a (small) patch to the CVS version of RegistryCooker.pm to do
> > this. Down side is that if new member data is added, it would then also
> > need to be added to the init sub.
>
> If you want to extend the object itself, what we can do is to provide a class
> method which will return the current size of the object. I suggest a method,
> so sub-classes can be further sub-classable.
>
> package A;
> use constant SIZE => 5;
> sub object_size { SIZE }
>
> package B;
> use constant EXTRA_SIZE => 2;
> use base qw(A);
> sub object_size { SUPER::object_size + EXTRA_SIZE);
>
> package C;
> use constant EXTRA_SIZE => 2;
> use base qw(B);
> sub object_size { SUPER::object_size + EXTRA_SIZE);
>
> etc...
>
> of course here we cast in stone the implementation of the object as an ARRAY,
> which is not so cool.
>
> Alternatively we can provide a function to create accessor methods, which will
> transparently handle internal changes.
>
> We could also use the 'fields' pragma, but than we get married to the hash
> internals, though apparently an optimized compiled time one. We need it
> working for 5.6.1+, is it working fine with 5.6.1?
>
> Pseudohashes are certainly out of question.
>
> __________________________________________________________________
> 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
--
Nathan Byrd <na...@byrd.net>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [patch] Changes to RegistryCooker for subclassing
Posted by Stas Bekman <st...@stason.org>.
Nathan Byrd wrote:
> All,
>
> To begin with, should proposed mod_perl patches go to
> dev@perl.apache.org? The documentation seemed a little unclear in this
> case (I decided to play it safe since I didn't run across any messages
> on the dev list from outside developers.)
dev@perl.apache.org would be the right place. Also help with the doc would be
*very* appreciated, I've started to write the initial doc, but it's a far away
from being useful.
> When I was converting Apache::PAR to work with mod_perl 2.x, one problem
> I had was with the way in which ModPerl::RegistryCooker stores member
> data. Any subclass of ModPerl::RegistryCooker (in my case, for
> Apache::PAR::RegistryCooker) that need to store their own module data
> have a problem - they need to pick an array element to store their data
> in. Because of the way in which ModPerl::RegistryCooker works
> currently, that means hardcoding an array index (because not all array
> members are created in new or init). Thus, in
> Apache::PAR::RegistryCooker I have a line similar to the following:
>
> use constant PARDATA => 8;
>
> This is not optimal, especially since this has already changed in the
> CVS version of RegistryCooker since I started working on it - luckily,
> to less members, not more :-)
>
> I propose a change to RegistryCooker.pm so that member data is always
> defined in the init sub, so that I could change the above line to
> something more like:
>
> use constant PARDATA => -1;
>
> and in my handler, push a new element on after new has been called.
> This would keep future changes to the RegistryCooker script from
> potentially breaking other modules which must store their own data as
> well.
>
> Below is a (small) patch to the CVS version of RegistryCooker.pm to do
> this. Down side is that if new member data is added, it would then also
> need to be added to the init sub.
If you want to extend the object itself, what we can do is to provide a class
method which will return the current size of the object. I suggest a method,
so sub-classes can be further sub-classable.
package A;
use constant SIZE => 5;
sub object_size { SIZE }
package B;
use constant EXTRA_SIZE => 2;
use base qw(A);
sub object_size { SUPER::object_size + EXTRA_SIZE);
package C;
use constant EXTRA_SIZE => 2;
use base qw(B);
sub object_size { SUPER::object_size + EXTRA_SIZE);
etc...
of course here we cast in stone the implementation of the object as an ARRAY,
which is not so cool.
Alternatively we can provide a function to create accessor methods, which will
transparently handle internal changes.
We could also use the 'fields' pragma, but than we get married to the hash
internals, though apparently an optimized compiled time one. We need it
working for 5.6.1+, is it working fine with 5.6.1?
Pseudohashes are certainly out of question.
__________________________________________________________________
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