[PATCH] GLOB to LVALUE assignment fix
Tassilo Von Parseval
attached you will find the revises patch against @22302 that extends an
LV to hold a complete GV. Differences to the patch draft include a
clean-up in dump.c and thus avoiding duplicate code.
I checked those places that do an explicit comparison between SvTYPEs
and changed them accordingly. With the previous patch, this
my $v = *GLOB; --$v;
would no longer produce the expected error 'Modification of a read-only
value attempted'. Now the error is back (note that we don't yet have a
test for that).
I added four tests to op/gv.t simply because I didn't find a better
place for them. The generic case
sub f { $_[0] = *GLOB }
f($var);
is tested, as is the case for a tied array-element (although this is
superseded by the above):
$tied[0] = *GLOB;
I didn't explicitely add a test for a tied hash-element.
Enjoy,
Tassilo
--
$_=q#",}])!JAPH!qq(tsuJ[{@"tnirp}3..0}_$;//::niam/s~=)]3[))_$-3(rellac(=_$({
pam{rekcahbus})(rekcah{lrePbus})(lreP{rehtonabus})!JAPH!qq(rehtona{tsuJbus#;
$_=reverse,s+(?<=sub).+q#q!'"qq.\t$&."'!#+sexisexiixesixeseg;y~\n~~dddd;eval
Tassilo Von Parseval
> attached you will find the revises patch against @22302 that extends an
> LV to hold a complete GV. Differences to the patch draft include a
> clean-up in dump.c and thus avoiding duplicate code.
This is now revision 2 of this patch (now against @22313), incorporating
Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
round. Other than that, it is identical.
I hope this is more to your liking.
Dave Mitchell
> This is now revision 2 of this patch (now against @22313), incorporating
> Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
> in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
> round. Other than that, it is identical.
Thanks, applied as change #22315.
I also applied the following change to optimise some of the tests for the
common case, and fixed up a few strange bits of indentation in your
original patch.
Dave.
--
Any [programming] language that doesn't occasionally surprise the
novice will pay for it by continually surprising the expert.
-- Larry Wall
Change 22316 by davem@davem-percy on 2004/02/17 18:23:27
Slight update to #22315 ($lval=*FOO); optimise the hot tests,
and fix some incorrect indentation
Affected files ...
... //depot/perl/pp.c#403 edit
... //depot/perl/pp_hot.c#344 edit
... //depot/perl/sv.c#720 edit
Differences ...
==== //depot/perl/pp.c#403 (text) ====
@@ -830,7 +830,7 @@
PP(pp_predec)
{
dSP;
- if (SvTYPE(TOPs) == SVt_PVGV || SvTYPE(TOPs) > SVt_PVLV)
+ if (SvTYPE(TOPs) >= SVt_PVGV && ! SvTYPE(TOPs) == SVt_PVLV)
DIE(aTHX_ PL_no_modify);
if (!SvREADONLY(TOPs) && SvIOK_notUV(TOPs) && !SvNOK(TOPs) && !SvPOK(TOPs)
&& SvIVX(TOPs) != IV_MIN)
@@ -847,7 +847,7 @@
PP(pp_postinc)
{
dSP; dTARGET;
- if (SvTYPE(TOPs) == SVt_PVGV || SvTYPE(TOPs) > SVt_PVLV)
+ if (SvTYPE(TOPs) >= SVt_PVGV && ! SvTYPE(TOPs) == SVt_PVLV)
DIE(aTHX_ PL_no_modify);
sv_setsv(TARG, TOPs);
if (!SvREADONLY(TOPs) && SvIOK_notUV(TOPs) && !SvNOK(TOPs) && !SvPOK(TOPs)
@@ -869,7 +869,7 @@
PP(pp_postdec)
{
dSP; dTARGET;
- if (SvTYPE(TOPs) == SVt_PVGV || SvTYPE(TOPs) > SVt_PVLV)
+ if (SvTYPE(TOPs) >= SVt_PVGV && ! SvTYPE(TOPs) == SVt_PVLV)
DIE(aTHX_ PL_no_modify);
sv_setsv(TARG, TOPs);
if (!SvREADONLY(TOPs) && SvIOK_notUV(TOPs) && !SvNOK(TOPs) && !SvPOK(TOPs)
==== //depot/perl/pp_hot.c#344 (text) ====
@@ -295,7 +295,7 @@
PP(pp_preinc)
{
dSP;
- if (SvTYPE(TOPs) == SVt_PVGV || SvTYPE(TOPs) > SVt_PVLV)
+ if (SvTYPE(TOPs) >= SVt_PVGV && ! SvTYPE(TOPs) == SVt_PVLV)
DIE(aTHX_ PL_no_modify);
if (!SvREADONLY(TOPs) && SvIOK_notUV(TOPs) && !SvNOK(TOPs) && !SvPOK(TOPs)
&& SvIVX(TOPs) != IV_MAX)
==== //depot/perl/sv.c#720 (text) ====
@@ -1455,11 +1455,11 @@
LvTARGLEN(sv) = 0;
LvTARG(sv) = 0;
LvTYPE(sv) = 0;
- GvGP(sv) = 0;
- GvNAME(sv) = 0;
- GvNAMELEN(sv) = 0;
- GvSTASH(sv) = 0;
- GvFLAGS(sv) = 0;
+ GvGP(sv) = 0;
+ GvNAME(sv) = 0;
+ GvNAMELEN(sv) = 0;
+ GvSTASH(sv) = 0;
+ GvFLAGS(sv) = 0;
break;
case SVt_PVAV:
SvANY(sv) = new_XPVAV();
@@ -3788,8 +3788,9 @@
if (dtype != SVt_PVGV) {
char *name = GvNAME(sstr);
STRLEN len = GvNAMELEN(sstr);
- if (dtype != SVt_PVLV) /* don't upgrade SVt_PVLV: it can hold a glob */
- sv_upgrade(dstr, SVt_PVGV);
+ /* don't upgrade SVt_PVLV: it can hold a glob */
+ if (dtype != SVt_PVLV)
+ sv_upgrade(dstr, SVt_PVGV);
sv_magic(dstr, dstr, PERL_MAGIC_glob, Nullch, 0);
GvSTASH(dstr) = (HV*)SvREFCNT_inc(GvSTASH(sstr));
GvNAME(dstr) = savepvn(name, len);
Tassilo Von Parseval
> On Tue, Feb 17, 2004 at 05:32:16PM +0100, Tassilo von Parseval wrote:
> > This is now revision 2 of this patch (now against @22313), incorporating
> > Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
> > in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
> > round. Other than that, it is identical.
>
> Thanks, applied as change #22315.
>
> I also applied the following change to optimise some of the tests for the
> common case, and fixed up a few strange bits of indentation in your
> original patch.
Thank you.
Speaking of indentation and whitespaces (and in the hope of doing it
properly from now on): My vim is configured to replace tabs with four
spaces. I was under the impression that this is the canonical way of
doing things.
You have now reverted spaces back to tabs, so should tabs be used in
patches?
Chip Salzenberg
> My vim is configured to replace tabs with four spaces. I was under
> the impression that this is the canonical way of doing things.
No. To the extent that any tab handling is canonical, the canonical
translation of tabs from the left margin is _eight_ spaces per tab.
This is pretty much what vim does with ":set tabstop=8".
Using the typical apparent indent of four spaces per level, whitespace
from the left margin will be zero or more tabs (eight spaces each)
followed by an optional four spaces (representing half a tab width).
--
Chip Salzenberg - a.k.a. - <ch...@pobox.com>
"I wanted to play hopscotch with the impenetrable mystery of existence,
but he stepped in a wormhole and had to go in early." // MST3K
Yitzchak Scott-Thoennes
> - if (SvTYPE(TOPs) == SVt_PVGV || SvTYPE(TOPs) > SVt_PVLV)
> + if (SvTYPE(TOPs) >= SVt_PVGV && ! SvTYPE(TOPs) == SVt_PVLV)
Umm, ! has higher precedence than ==, so you want
if (SvTYPE(TOPs) >= SVt_PVGV && SvTYPE(TOPs) != SVt_PVLV)
here and in the other places.
John Peacock
> Speaking of indentation and whitespaces (and in the hope of doing it
> properly from now on): My vim is configured to replace tabs with four
> spaces. I was under the impression that this is the canonical way of
> doing things.
Not exactly.
>
> You have now reverted spaces back to tabs, so should tabs be used in
> patches?
>
Initial indent is 4 spaces. Groups of 8 spaces are converted to a tab. Try
using this:
http://backpan.cpan.org/authors/id/T/TO/TOMC/scripts/toms.exrc.gz
and you will get what is basically the default Perl source format...
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
Rafael Garcia-Suarez
> According to Tassilo von Parseval:
> > My vim is configured to replace tabs with four spaces. I was under
> > the impression that this is the canonical way of doing things.
>
> No. To the extent that any tab handling is canonical, the canonical
> translation of tabs from the left margin is _eight_ spaces per tab.
> This is pretty much what vim does with ":set tabstop=8".
or, more verbosely, and more vim-ly,
:set ts=8 sts=4 sw=4 noet
There was some talk on IRC about adding an equivalent modeline to all
C files. Might be a good idea.
Rafael Garcia-Suarez
> On Tue, Feb 17, 2004 at 05:32:16PM +0100, Tassilo von Parseval wrote:
> > This is now revision 2 of this patch (now against @22313), incorporating
> > Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
> > in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
> > round. Other than that, it is identical.
>
> Thanks, applied as change #22315.
>
> I also applied the following change to optimise some of the tests for the
> common case, and fixed up a few strange bits of indentation in your
> original patch.
I'm still a bit worried about it ; notably, will XS modules break ? and
if so, how many of them ?
This deserves, at least, a big mention in perldelta (although I'm not
sure how to word it.)
Dave Mitchell
Guilty as charged :-(
Fixed by 22317.
Interestingly it didn't fail any tests, becuase its very hard to
get as far as pp_*inc without having something valid on the stack.
And even if you do (eg $glob=*FOO; $glob++), the error is caught further
on anyway.
--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom - Arcadia
Dave Mitchell
> Dave Mitchell <da...@fdisolutions.com> wrote:
>
> > On Tue, Feb 17, 2004 at 05:32:16PM +0100, Tassilo von Parseval wrote:
> > > This is now revision 2 of this patch (now against @22313), incorporating
> > > Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
> > > in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
> > > round. Other than that, it is identical.
> >
> > Thanks, applied as change #22315.
> >
> > I also applied the following change to optimise some of the tests for the
> > common case, and fixed up a few strange bits of indentation in your
> > original patch.
>
> I'm still a bit worried about it ; notably, will XS modules break ? and
> if so, how many of them ?
Well, none of the core XS modules needed fixing, which is a good sign.
The re-ordering of SVf_* constants really only has two effects:
1) PVGV is now before PVLV. Given how generally obscure LVs are and how
under-doumented they are, I think it's fairly remote that any XS code is
repling on this ordering.
2) PVGV is now before PVAV,PVHV and PVCV. Difficult to think why any XS
code would rely on this ordering, as there isn't any direct inheritance
relationship between them.
Still, stranger things have been known.
> This deserves, at least, a big mention in perldelta (although I'm not
> sure how to word it.)
First draft (the head1 bit is just cut+pasted from 5.9.0delta):
=head1 Changed Internals
These news matter to you only if you either write XS code or like to
know about or hack Perl internals (using Devel::Peek or any of the
C<B::> modules counts), or like to run Perl with the C<-D> option.
=head2 Reordering of SVf_ constants
The relative ordering of constants that define the various types of C<SV>
have changed; in particular, C<SVf_PVGV> has been moved ahead of C<SVF_PVLV>,
C<SVF_PVAV>, C<SVF_PVHV> and C<SVF_PVCV>. This is unlikely to make any
diffrence unless you have code that explicitly makes assumptions about that
ordering.
--
Counsellor Troi states something other than the blindingly obvious.
-- Things That Never Happen in "Star Trek" #16
Rafael Garcia-Suarez
> =head2 Reordering of SVf_ constants
>
> The relative ordering of constants that define the various types of C<SV>
> have changed; in particular, C<SVf_PVGV> has been moved ahead of C<SVF_PVLV>,
> C<SVF_PVAV>, C<SVF_PVHV> and C<SVF_PVCV>. This is unlikely to make any
> diffrence unless you have code that explicitly makes assumptions about that
> ordering.
OK, applied as #22321 with some tweaks of my own.
Tassilo Von Parseval
> On Tue, Feb 17, 2004 at 09:15:55PM +0100, Rafael Garcia-Suarez wrote:
> > Dave Mitchell <da...@fdisolutions.com> wrote:
> >
> > > On Tue, Feb 17, 2004 at 05:32:16PM +0100, Tassilo von Parseval wrote:
> > > > This is now revision 2 of this patch (now against @22313), incorporating
> > > > Yitzchack's comments. ext/B/B.pm sets @B::PVLV::ISA accordingly and does
> > > > in fact move SVt_PVGV before SVt_PVLV instead of doing it the other way
> > > > round. Other than that, it is identical.
> > >
> > > Thanks, applied as change #22315.
> > >
> > > I also applied the following change to optimise some of the tests for the
> > > common case, and fixed up a few strange bits of indentation in your
> > > original patch.
> >
> > I'm still a bit worried about it ; notably, will XS modules break ? and
> > if so, how many of them ?
>
> Well, none of the core XS modules needed fixing, which is a good sign.
Well, none except B itself that used SvTYPE(sv) as index of an array to
look up the symbolic names of the svtype enum.
Potentially, any module that e.g. does a
if (SvTYPE(sv) < SVt_GV)
and expects an AV or HV to pass this test is at risk. I expect that
those are modules that directly deal with perl's internals so candidates
are likely to be found in the Devel:: or B:: namespace.
Some of the big XS modules could serve as a good test, such as DBI or
Tk. I just tried DBI and all tests pass except one which almost
certainly doesn't have anything to do with SV types (t/41prof_dump.t,
test 6). I can't try Tk because its tests do not even pass for any of my
5.8.x perls.
Nick Ing-Simmons
>> > I'm still a bit worried about it ; notably, will XS modules break ? and
>> > if so, how many of them ?
>>
>> Well, none of the core XS modules needed fixing, which is a good sign.
>
>Well, none except B itself that used SvTYPE(sv) as index of an array to
>look up the symbolic names of the svtype enum.
>
>Potentially, any module that e.g. does a
>
> if (SvTYPE(sv) < SVt_GV)
Main use or inequalities in XS will be for this sort of thing:
MAGIC *mg = (SvTYPE(obj) >= SVt_PVMG) ? mg_find(obj,PERL_MAGIC_ext) : NULL;
I assume that is still valid?
>
>Some of the big XS modules could serve as a good test, such as DBI or
>Tk. I just tried DBI and all tests pass except one which almost
>certainly doesn't have anything to do with SV types (t/41prof_dump.t,
>test 6). I can't try Tk because its tests do not even pass for any of my
>5.8.x perls.
Tk804 should work okay on 5.8.*
Snag is some of tests are over sensative to Window manager and fonts
available.
>
>Tassilo
Dave Mitchell
> Main use or inequalities in XS will be for this sort of thing:
>
> MAGIC *mg = (SvTYPE(obj) >= SVt_PVMG) ? mg_find(obj,PERL_MAGIC_ext) : NULL;
>
> I assume that is still valid?
Yes.
--
Wesley Crusher gets beaten up by his classmates for being a smarmy git,
and consequently has a go at making some friends of his own age for a
change.
-- Things That Never Happen in "Star Trek" #18