Skip Menu |
 

This queue is for tickets about the CGI-Session CPAN distribution.

Report information
The Basics
Id: 21784
Status: resolved
Priority: 0/
Queue: CGI-Session

People
Owner: Nobody in particular
Requestors: fenlisesi [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Unimportant
Broken in: 4.14
Fixed in: (no value)



Subject: notabug: some minor cleanup in sub new()
MIME-Version: 1.0
X-Mailer: MIME-tools 5.418 (Entity 5.418)
X-RT-Original-Encoding: utf-8
Content-Type: multipart/mixed; boundary="----------=_1159573355-8325-6"
Content-Length: 0
Content-Type: text/plain; charset="utf8"
Content-Disposition: inline
Content-Transfer-Encoding: binary
Content-Length: 669
Download (untitled) / with headers
text/plain 669b
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
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----------=_1159573355-8325-5"
X-Mailer: MIME-tools 5.418 (Entity 5.418)
Content-Length: 0
Content-Type: text/plain; charset="utf8"
Content-Disposition: inline
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 0
Content-Type: text/x-patch; name="CGI-Session-new-4.14.patch"
Content-Disposition: inline; filename="CGI-Session-new-4.14.patch"
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: ascii
Content-Length: 4001
--- 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; }
MIME-Version: 1.0
X-Spam-Status: No, hits=-2.6 required=8.0 tests=BAYES_00
In-Reply-To: <rt-3.6.HEAD-8325-1159573358-731.21784-4-0 [...] rt.cpan.org>
Received-SPF: neutral (x1.develooper.com: local policy)
References: <RT-Ticket-21784 [...] rt.cpan.org> <rt-3.6.HEAD-8325-1159573358-731.21784-4-0 [...] rt.cpan.org>
Content-Type: text/plain; charset="utf-8"
X-RT-Original-Encoding: ISO-8859-1
Received: from la.mx.develooper.com (x1.develooper.com [63.251.223.170]) by diesel.bestpractical.com (Postfix) with SMTP id 45DC54D8141 for <bug-CGI-Session [...] rt.cpan.org>; Tue, 3 Oct 2006 10:04:30 -0400 (EDT)
Received: (qmail 18212 invoked by alias); 3 Oct 2006 14:04:29 -0000
Received: from mail.parallax.ws (HELO mail.parallax.ws) (12.161.104.4) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Tue, 03 Oct 2006 07:04:24 -0700
Received: (qmail 47637 invoked by uid 89); 3 Oct 2006 14:04:16 -0000
Received: by simscan 1.2.0 ppid: 47615, pid: 47617, t: 3.1503s scanners: clamav: 0.87/m:34/d:1109 spam: 3.1.0
Received: from unknown (HELO ?12.161.105.182?) (12.161.105.182) by 0 with SMTP; 3 Oct 2006 14:04:13 -0000
Delivered-To: cpan-bug+cgi-session [...] diesel.bestpractical.com
Subject: Re: [rt.cpan.org #21784] notabug: some minor cleanup in sub new()
User-Agent: Thunderbird 1.5 (X11/20060405)
Return-Path: <mark [...] summersault.com>
X-Spam-Check-BY: la.mx.develooper.com
X-Original-To: bug-CGI-Session [...] rt.cpan.org
X-Old-Spam-Status: No, score=0.0 required=5.0 tests=AWL autolearn=disabled version=3.1.0
Date: Tue, 03 Oct 2006 10:02:40 -0400
X-Spam-Level:
Message-Id: <45226D80.2060502 [...] summersault.com>
X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on ns1.parallax.ws
To: bug-CGI-Session [...] rt.cpan.org
X-Enigmail-Version: 0.94.1.0
Content-Transfer-Encoding: 7bit
From: Mark Stosberg <mark [...] summersault.com>
X-RT-Original-Encoding: utf-8
RT-Message-ID: <rt-3.6.HEAD-15931-1159884278-1285.21784-0-0 [...] rt.cpan.org>
Content-Length: 551
Download (untitled) / with headers
text/plain 551b
Ali ISIK via RT wrote: Show quoted text
> > 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. >
Reviewed, and applied to svn, passes 'make test'. Mark


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.