Re: Patch to t/modules/request.t

[Doug MacEachern]

On Thu, 31 Aug 2000, Ken Williams wrote:

> [EMAIL PROTECTED] (Doug MacEachern) wrote:
> >this thread is pretty large, i'm sure the fix is nice and small, anybody
> >have a patch with the final solution?  thanks.
> 
> Sounds so ominous... =)

didn't mean it to, i'm just buried and looking for any shortcuts out,
thanks :)

Re: Patch to t/modules/request.t

[Ken Williams]

[EMAIL PROTECTED] (Doug MacEachern) wrote:
>this thread is pretty large, i'm sure the fix is nice and small, anybody
>have a patch with the final solution?  thanks.

Sounds so ominous... =)

===
RCS file: /home/cvspublic/modperl/t/modules/request.t,v
retrieving revision 1.8
diff -u -r1.8 request.t
--- request.t   2000/05/12 03:43:24 1.8
+++ request.t   2000/08/31 05:16:19
@@ -125,7 +125,7 @@
 my $lines = 0;
 local *FH;
 open FH, $file or die "open $file $!";
-++$lines while (my $dummy = );
+++$lines while defined ;
 close FH;
 my(@headers);
 if ($Is_dougm) {
===


  ------
  Ken Williams Last Bastion of Euclidity
  [EMAIL PROTECTED]The Math Forum

Re: Patch to t/modules/request.t

[Doug MacEachern]

this thread is pretty large, i'm sure the fix is nice and small, anybody
have a patch with the final solution?  thanks.

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 27, 2000 at 12:17:41 -0500, Ken Williams twiddled the keys to say:
> [EMAIL PROTECTED] (Rick Myers) wrote:
> >I would lean towards the second one since upload_test() is called
> >similarly from three different places within request.t.
> >
> >The reasoning behind suggesting `while defined ' was that the
> >interaction with $_ appears to be unintentional, at least within the
> >scope of upload_test(). We're basically throwing away the content of the
> >file read anyway, so why not just eliminate the undesired setting of $_
> >altogether. Also, there's no need to localize $_ since we aren't
> >referencing it to do anything.
> >
> >  for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
> >  upload_test($_);
> >  }
> >
> >  sub upload_test {
> >  my $podfile = shift || "func";
> >  ...
> >  ++$lines while defined ;
> >  }
> >
> >FWIW, adding defined() was the perldiag-suggested fix for the problem
> >Ken was seeing. It also happens to fix the read-only error quite nicely
> >in this particular situation.
> 
> 
> Sounds good to me.  But I still can't reproduce the original read-only error. 
> The following code runs just fine for me under 5.004 and 5.005:
> 
>@binary = "blah";
>for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
>  upload_test($_);
>}
> 
>sub upload_test {
>  ++$lines while ;
>}
> 
> The only funny business is that  clobbers $_, so that the various
> values iterated through in the for() loop all eventually get set to
> undef.

Aha!

perl5.00503 -e 'test($_) for qw(a b c); print "OK\n"; sub test {$_=0}'
OK

perl5.6.0 -e 'test($_) for qw(a b c); print "OK\n"; sub test {$_=0}'
Modification of a read-only value attempted at -e line 1.

Chalk one up for 5.6 I guess.

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Re: Patch to t/modules/request.t

[Ken Williams]

[EMAIL PROTECTED] (Rick Myers) wrote:
>I would lean towards the second one since upload_test() is called
>similarly from three different places within request.t.
>
>The reasoning behind suggesting `while defined ' was that the
>interaction with $_ appears to be unintentional, at least within the
>scope of upload_test(). We're basically throwing away the content of the
>file read anyway, so why not just eliminate the undesired setting of $_
>altogether. Also, there's no need to localize $_ since we aren't
>referencing it to do anything.
>
>  for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
>  upload_test($_);
>  }
>
>  sub upload_test {
>  my $podfile = shift || "func";
>  ...
>  ++$lines while defined ;
>  }
>
>FWIW, adding defined() was the perldiag-suggested fix for the problem
>Ken was seeing. It also happens to fix the read-only error quite nicely
>in this particular situation.


Sounds good to me.  But I still can't reproduce the original read-only error. 
The following code runs just fine for me under 5.004 and 5.005:

   @binary = "blah";
   for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
 upload_test($_);
   }

   sub upload_test {
 ++$lines while ;
   }

The only funny business is that  clobbers $_, so that the various
values iterated through in the for() loop all eventually get set to
undef.


  ------
  Ken Williams Last Bastion of Euclidity
  [EMAIL PROTECTED]The Math Forum

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 26, 2000 at 14:07:26 +0200, Stas Bekman twiddled the keys to say:
> On Fri, 25 Aug 2000, Rick Myers wrote:
> 
> > On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> > > [EMAIL PROTECTED] (Rick Myers) wrote:
> > > >On Aug 24, 2000 at 01:15:57 -0500, Ken Williams twiddled the keys to say:
> > > >> The following patch eliminates a warning during 'make test' about 'Value
> > > >> of  construct can be "0";'.  No biggie, but it should be fixed.
> > > >> 
> > > >> 
> > > >> --- t/modules/request.t 2000/05/12 03:43:24 1.8
> > > >> +++ t/modules/request.t 2000/08/24 06:07:40
> > > >> @@ -125,7 +125,7 @@
> > > >>  my $lines = 0;
> > > >>  local *FH;
> > > >>  open FH, $file or die "open $file $!";
> > > >> -++$lines while (my $dummy = );
> > > >> +++$lines while ;
> > > >>  close FH;
> > > >>  my(@headers);
> > > >>  if ($Is_dougm) {
> > > >> 
> > > >
> > > >This reverses a previous patch that fixes a fatal 'Modification of a
> > > >read-only value attempted at modules/request.t line 128', which returns
> > > >with this patch.
> > > >
> > > >See if this one finds us a happy median...
> > > >
> > > >--- t/modules/request.t~ Thu Aug 24 18:24:39 2000
> > > >+++ t/modules/request.t  Thu Aug 24 18:41:22 2000
> > > >@@ -125,7 +125,7 @@
> > > > my $lines = 0;
> > > > local *FH;
> > > > open FH, $file or die "open $file $!";
> > > >-++$lines while (my $dummy = );
> > > >+++$lines while defined ;
> > > > close FH;
> > > > my(@headers);
> > > > if ($Is_dougm) {
> > > 
> > > 
> > > THAT's weird - what in the world is the read-only value that's being
> > > modified?  
> > 
> > It's $_.
> > 
> > Here's the relevant code from request.t to illustrate...
> > 
> >   for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
> >   upload_test($_);
> >   }
> > 
> >   sub upload_test {
> >   ++$lines while ;
> >   }
> 
> The real fix should be either:
> 
>   for my $file ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
>   upload_test($file);
>   }
> 
>   sub upload_test {
>   ++$lines while ;
>   }
> 
> or
> 
>   for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
>   upload_test($_);
>   }
> 
>   sub upload_test {
>   local $_;
>   ++$lines while ;
>   }

I would lean towards the second one since upload_test() is called
similarly from three different places within request.t.

The reasoning behind suggesting `while defined ' was that the
interaction with $_ appears to be unintentional, at least within the
scope of upload_test(). We're basically throwing away the content of the
file read anyway, so why not just eliminate the undesired setting of $_
altogether. Also, there's no need to localize $_ since we aren't
referencing it to do anything.

  for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
  upload_test($_);
  }

  sub upload_test {
  my $podfile = shift || "func";
  ...
  ++$lines while defined ;
  }

FWIW, adding defined() was the perldiag-suggested fix for the problem
Ken was seeing. It also happens to fix the read-only error quite nicely
in this particular situation.

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Re: Patch to t/modules/request.t

[Stas Bekman]

On Fri, 25 Aug 2000, Rick Myers wrote:

> On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> > [EMAIL PROTECTED] (Rick Myers) wrote:
> > >On Aug 24, 2000 at 01:15:57 -0500, Ken Williams twiddled the keys to say:
> > >> The following patch eliminates a warning during 'make test' about 'Value
> > >> of  construct can be "0";'.  No biggie, but it should be fixed.
> > >> 
> > >> 
> > >> --- t/modules/request.t 2000/05/12 03:43:24 1.8
> > >> +++ t/modules/request.t 2000/08/24 06:07:40
> > >> @@ -125,7 +125,7 @@
> > >>  my $lines = 0;
> > >>  local *FH;
> > >>  open FH, $file or die "open $file $!";
> > >> -++$lines while (my $dummy = );
> > >> +++$lines while ;
> > >>  close FH;
> > >>  my(@headers);
> > >>  if ($Is_dougm) {
> > >> 
> > >
> > >This reverses a previous patch that fixes a fatal 'Modification of a
> > >read-only value attempted at modules/request.t line 128', which returns
> > >with this patch.
> > >
> > >See if this one finds us a happy median...
> > >
> > >--- t/modules/request.t~   Thu Aug 24 18:24:39 2000
> > >+++ t/modules/request.tThu Aug 24 18:41:22 2000
> > >@@ -125,7 +125,7 @@
> > > my $lines = 0;
> > > local *FH;
> > > open FH, $file or die "open $file $!";
> > >-++$lines while (my $dummy = );
> > >+++$lines while defined ;
> > > close FH;
> > > my(@headers);
> > > if ($Is_dougm) {
> > 
> > 
> > THAT's weird - what in the world is the read-only value that's being
> > modified?  
> 
> It's $_.
> 
> Here's the relevant code from request.t to illustrate...
> 
>   for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
>   upload_test($_);
>   }
> 
>   sub upload_test {
>   ++$lines while ;
>   }

The real fix should be either:

  for my $file ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
  upload_test($file);
  }

  sub upload_test {
  ++$lines while ;
  }

or

  for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
  upload_test($_);
  }

  sub upload_test {
  local $_;
  ++$lines while ;
  }

The really one and the only correct one is of course:

  for my $file ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
  upload_test($file);
  }

  sub upload_test {
  local $_;
  ++$lines while ;
  }

You are trying to workaround the problem. It's always becomes very subtle
to find bug when you do:

  for (1..5) { function($_) }

Because you never know whether the function itself is using $_ and
modifying it. In this situation you are lucky that the list you pass to
the for is readonly -- perl reports a problem. A much worth thing happen
when the variable that you pass to for() is a real array:

  for (@array) { function($_) }

$_ is aliased to the real variables in @array -- it's not a copy!!! So if
function() alters $_, your array gets changed!!! This is a side effect
which can byte you very badly.

Therefore, if you are a good perl programmer and want your code to be
clean, when you write subroutines which work with $_, always start them
with:

  local $_;

If you write for() and similar loops that alias $_ to the real variables
in the list, *always* do:

  for my $val (@array) { foo($val) }

use $_ only if the loop doesn't call any function unless you are sure that
the function declares 'local $_';. The following is a perfect snippet of
code:

  my $count = 0;
  for (@values){
$count += $_;
  }

> 
> The hint is that `while ' will try to set $_ to the value of each
> line read.
> 
> Rick Myers[EMAIL PROTECTED]
> 
> The Feynman Problem   1) Write down the problem.
> Solving Algorithm 2) Think real hard.
>   3) Write down the answer.
> 



_
Stas Bekman  JAm_pH --   Just Another mod_perl Hacker
http://stason.org/   mod_perl Guide  http://perl.apache.org/guide 
mailto:[EMAIL PROTECTED]   http://apachetoday.com http://jazzvalley.com
http://singlesheaven.com http://perlmonth.com   perl.org   apache.org

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 25, 2000 at 21:25:59 -0400, Rick Myers twiddled the keys to say:
> On Aug 25, 2000 at 21:02:32 -0400, Billy Donahue twiddled the keys to say:
> > -BEGIN PGP SIGNED MESSAGE-
> > Hash: SHA1
> > 
> > On Fri, 25 Aug 2000, Rick Myers wrote:
> > 
> > > From: Rick Myers <[EMAIL PROTECTED]>
> > > 
> > > On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> > > > [EMAIL PROTECTED] (Rick Myers) wrote:
> > > > >
> > > > >+++$lines while defined ;
> > > > 
> > > > THAT's weird - what in the world is the read-only value that's being
> > > > modified?  
> > > 
> > > It's $_.
> > 
> > And why is $_ read-only?
> 
> I'm clueless about the perlguts involved here, but I'm thinking
> ultimately because it's global...
> 
> perl -e 'test($_) for qw/a b c/; print "OK\n"; sub test {$_=0}'
> Modification of a read-only value attempted at -e line 1.
> 
> perl -e 'test($_) for qw/a b c/; print "OK\n"; sub test {local $_=0}'
> OK

*Ugh*  I guess I have to correct myself here...

As it turns out, the perldiag page is correct. In the above examples $_
can't be modified because it "points" to constant values (the qw
construct).

perl -e 'for ($_=0;$_<5;$_++) {test($_)} print "OK\n"; sub test {$_=0}'


perl -e 'for ($_=0;$_<5;$_++) {test($_)} print "OK\n"; sub test {local $_=0}'
OK

Nevertheless, the real problem in request.t is still the `while '.

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 25, 2000 at 21:02:32 -0400, Billy Donahue twiddled the keys to say:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
> 
> On Fri, 25 Aug 2000, Rick Myers wrote:
> 
> > From: Rick Myers <[EMAIL PROTECTED]>
> > 
> > On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> > > [EMAIL PROTECTED] (Rick Myers) wrote:
> > > >
> > > >+++$lines while defined ;
> > > 
> > > THAT's weird - what in the world is the read-only value that's being
> > > modified?  
> > 
> > It's $_.
> 
> And why is $_ read-only?

I'm clueless about the perlguts involved here, but I'm thinking
ultimately because it's global...

perl -e 'test($_) for qw/a b c/; print "OK\n"; sub test {$_=0}'
Modification of a read-only value attempted at -e line 1.

perl -e 'test($_) for qw/a b c/; print "OK\n"; sub test {local $_=0}'
OK

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Re: Patch to t/modules/request.t

[Billy Donahue]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Fri, 25 Aug 2000, Rick Myers wrote:

> From: Rick Myers <[EMAIL PROTECTED]>
> 
> On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> > [EMAIL PROTECTED] (Rick Myers) wrote:
> > >
> > >+++$lines while defined ;
> > 
> > THAT's weird - what in the world is the read-only value that's being
> > modified?  
> 
> It's $_.

And why is $_ read-only?

- --
"The Funk, the whole Funk, and nothing but the Funk."
Billy Donahue 
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.0.2 (GNU/Linux)
Comment: pgpenvelope 2.9.0 - http://pgpenvelope.sourceforge.net/

iD8DBQE5pxcs+2VvpwIZdF0RAkWQAJ99KD28UFuU+atZL1UueucMyHN3ggCfSaCf
QvX7iCGkTVwBJqp6zamplwk=
=A+tD
-END PGP SIGNATURE-

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 24, 2000 at 23:15:15 -0500, Ken Williams twiddled the keys to say:
> [EMAIL PROTECTED] (Rick Myers) wrote:
> >On Aug 24, 2000 at 01:15:57 -0500, Ken Williams twiddled the keys to say:
> >> The following patch eliminates a warning during 'make test' about 'Value
> >> of  construct can be "0";'.  No biggie, but it should be fixed.
> >> 
> >> 
> >> --- t/modules/request.t 2000/05/12 03:43:24 1.8
> >> +++ t/modules/request.t 2000/08/24 06:07:40
> >> @@ -125,7 +125,7 @@
> >>  my $lines = 0;
> >>  local *FH;
> >>  open FH, $file or die "open $file $!";
> >> -++$lines while (my $dummy = );
> >> +++$lines while ;
> >>  close FH;
> >>  my(@headers);
> >>  if ($Is_dougm) {
> >> 
> >
> >This reverses a previous patch that fixes a fatal 'Modification of a
> >read-only value attempted at modules/request.t line 128', which returns
> >with this patch.
> >
> >See if this one finds us a happy median...
> >
> >--- t/modules/request.t~ Thu Aug 24 18:24:39 2000
> >+++ t/modules/request.t  Thu Aug 24 18:41:22 2000
> >@@ -125,7 +125,7 @@
> > my $lines = 0;
> > local *FH;
> > open FH, $file or die "open $file $!";
> >-++$lines while (my $dummy = );
> >+++$lines while defined ;
> > close FH;
> > my(@headers);
> > if ($Is_dougm) {
> 
> 
> THAT's weird - what in the world is the read-only value that's being
> modified?  

It's $_.

Here's the relevant code from request.t to illustrate...

  for ( qw(perlfunc.pod perlpod.pod perlxs.pod), @binary) {
  upload_test($_);
  }

  sub upload_test {
  ++$lines while ;
  }

The hint is that `while ' will try to set $_ to the value of each
line read.

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Re: Patch to t/modules/request.t

[Ken Williams]

[EMAIL PROTECTED] (Rick Myers) wrote:
>On Aug 24, 2000 at 01:15:57 -0500, Ken Williams twiddled the keys to say:
>> The following patch eliminates a warning during 'make test' about 'Value
>> of  construct can be "0";'.  No biggie, but it should be fixed.
>> 
>> 
>> --- t/modules/request.t 2000/05/12 03:43:24 1.8
>> +++ t/modules/request.t 2000/08/24 06:07:40
>> @@ -125,7 +125,7 @@
>>  my $lines = 0;
>>  local *FH;
>>  open FH, $file or die "open $file $!";
>> -++$lines while (my $dummy = );
>> +++$lines while ;
>>  close FH;
>>  my(@headers);
>>  if ($Is_dougm) {
>> 
>
>This reverses a previous patch that fixes a fatal 'Modification of a
>read-only value attempted at modules/request.t line 128', which returns
>with this patch.
>
>See if this one finds us a happy median...
>
>--- t/modules/request.t~   Thu Aug 24 18:24:39 2000
>+++ t/modules/request.tThu Aug 24 18:41:22 2000
>@@ -125,7 +125,7 @@
> my $lines = 0;
> local *FH;
> open FH, $file or die "open $file $!";
>-++$lines while (my $dummy = );
>+++$lines while defined ;
> close FH;
> my(@headers);
> if ($Is_dougm) {


THAT's weird - what in the world is the read-only value that's being
modified?  


  ------
  Ken Williams Last Bastion of Euclidity
  [EMAIL PROTECTED]The Math Forum

Re: Patch to t/modules/request.t

[Rick Myers]

On Aug 24, 2000 at 01:15:57 -0500, Ken Williams twiddled the keys to say:
> The following patch eliminates a warning during 'make test' about 'Value
> of  construct can be "0";'.  No biggie, but it should be fixed.
> 
> 
> --- t/modules/request.t 2000/05/12 03:43:24 1.8
> +++ t/modules/request.t 2000/08/24 06:07:40
> @@ -125,7 +125,7 @@
>  my $lines = 0;
>  local *FH;
>  open FH, $file or die "open $file $!";
> -++$lines while (my $dummy = );
> +++$lines while ;
>  close FH;
>  my(@headers);
>  if ($Is_dougm) {
> 

This reverses a previous patch that fixes a fatal 'Modification of a
read-only value attempted at modules/request.t line 128', which returns
with this patch.

See if this one finds us a happy median...

--- t/modules/request.t~Thu Aug 24 18:24:39 2000
+++ t/modules/request.t Thu Aug 24 18:41:22 2000
@@ -125,7 +125,7 @@
 my $lines = 0;
 local *FH;
 open FH, $file or die "open $file $!";
-++$lines while (my $dummy = );
+++$lines while defined ;
 close FH;
 my(@headers);
 if ($Is_dougm) {

Rick Myers[EMAIL PROTECTED]

The Feynman Problem   1) Write down the problem.
Solving Algorithm 2) Think real hard.
  3) Write down the answer.

Patch to t/modules/request.t

[Ken Williams]

The following patch eliminates a warning during 'make test' about 'Value
of  construct can be "0";'.  No biggie, but it should be fixed.


--- t/modules/request.t 2000/05/12 03:43:24 1.8
+++ t/modules/request.t 2000/08/24 06:07:40
@@ -125,7 +125,7 @@
 my $lines = 0;
 local *FH;
 open FH, $file or die "open $file $!";
-++$lines while (my $dummy = );
+++$lines while ;
 close FH;
 my(@headers);
 if ($Is_dougm) {



Also: this test tests Apache::Request, but it's only run if the current
user is 'dougm'.  Is it time to let this test out of the cat-bag?  =)