| Subject: | notabug: some minor cleanup in sub new() |
In the attached patch, I suggest a few non-semantic changes
in new() that
(1) reduce the width of the source code
(2) unpack the argv asap into @args
(3) possibly reduce the number of hash accesses
(did not benchmark this)
(4) possibly improve readability
The patch is against 4-14. It passes all the non-skipped
tests that come with the 4-14 distribution on my machine,
but please examine it closely anyway, as nobody reviewed it.
$CGI::Session::VERSION = '4.14';
This is perl, v5.8.8 built for i686-linux-thread-multi
Linux version 2.6.11.4-21.14-default (geeko@buildhost)
(gcc version 3.3.5 20050117 (prerelease) (SUSE Linux))
#1 Thu Aug 24 09:51:41 UTC 2006
| Subject: | CGI-Session-new-4.14.patch |
--- Session-original.pm 2006-06-11 15:13:35.000000000 +0300
+++ Session-edited.pm 2006-09-30 02:10:51.000000000 +0300
@@ -26,40 +26,56 @@
}
sub new {
- my $class = shift;
+ my ($class, @args) = @_;
- # If called as object method as in $session->new()...
my $self;
- if ( ref $class ) {
- $self = bless { %$class }, ref($class);
- $class = ref($class);
+ if (ref $class) {
+ #
+ # Called as an object method as in $session->new()...
+ #
+ $self = bless { %$class }, ref( $class );
+ $class = ref $class;
$self->_reset_status();
-
- # Object may still have public data associated with it, but we don't care about that,
- # since we want to leave that to the client's disposal. However, if new() was requested on
- # an expired session, we already know that '_DATA' table is empty, since it was the
- # job of flush() to empty '_DATA' after deleting. How do we know flush() was already
- # called on an expired session? Because load() - constructor always calls flush()
- # on all to-be expired sessions
- } else {
- defined($self = $class->load( @_ ))
- or return $class->set_error( "new(): failed: " . $class->errstr );
- }
-
- # Absence of '_SESSION_ID' can only signal:
- # * expired session
- # Because load() - constructor is required to empty contents of _DATA - table
- # * unavailable session
- # Such sessions are the ones that don't exist on datastore, but requested by client
- # * new sessions
- # When no specific session is requested to be loaded
- unless ( $self->{_DATA}->{_SESSION_ID} ) {
- $self->{_DATA}->{_SESSION_ID} = $self->_id_generator()->generate_id($self->{_DRIVER_ARGS}, $self->{_CLAIMED_ID});
- unless ( defined $self->{_DATA}->{_SESSION_ID} ) {
- return $self->set_error( "Couldn't generate new SID" );
+ #
+ # Object may still have public data associated with it, but we
+ # don't care about that, since we want to leave that to the
+ # client's disposal. However, if new() was requested on an
+ # expired session, we already know that '_DATA' table is
+ # empty, since it was the job of flush() to empty '_DATA'
+ # after deleting. How do we know flush() was already called on
+ # an expired session? Because load() - constructor always
+ # calls flush() on all to-be expired sessions
+ #
+ }
+ else {
+ #
+ # Called as a class method as in CGI::Session->new()
+ #
+ $self = $class->load( @args );
+ if (not defined $self) {
+ return $class->set_error( "new(): failed: " . $class->errstr );
+ }
+ }
+ my $dataref = $self->{_DATA};
+ unless ($dataref->{_SESSION_ID}) {
+ #
+ # Absence of '_SESSION_ID' can only signal:
+ # * Expired session: Because load() - constructor is required to
+ # empty contents of _DATA - table
+ # * Unavailable session: Such sessions are the ones that don't
+ # exist on datastore, but are requested by client
+ # * New session: When no specific session is requested to be loaded
+ #
+ my $id = $self->_id_generator()->generate_id(
+ $self->{_DRIVER_ARGS},
+ $self->{_CLAIMED_ID}
+ );
+ unless (defined $id) {
+ return $self->set_error( "Couldn't generate new SESSION-ID" );
}
- $self->{_DATA}->{_SESSION_CTIME} = $self->{_DATA}->{_SESSION_ATIME} = time();
- $self->_set_status(STATUS_NEW);
+ $dataref->{_SESSION_ID} = $id;
+ $dataref->{_SESSION_CTIME} = $dataref->{_SESSION_ATIME} = time();
+ $self->_set_status( STATUS_NEW );
}
return $self;
}