parent.pm at http://corion.net/perl-dev

Hello Porters,

I've followed the p5p discussion on base.pm and after
discussions with 
Anno and Bart wrote parent.pm ([1], [2]). So far I haven't
uploaded it 
onto CPAN as it's an all-lowercase module name, and I'm
waiting for the 
"go" from p5p.

The feature set is as follows:

a) Module loading is done via

   require $file

b) Classes can be inherited from via other modules by using
an array 
reference [ $class => $module ]:

   use parent [ Tie::StdHash => Tie::Hash ];

c) Classes can be inherited from without loading a file by
passing a 
false value as the module:

   use parent [ My::InnerClass => undef ];

All applicable tests from base.pm pass with the appropriate

modifications and I've added a test for inheriting from
classes like E'Mail.

I'd very much like this to go into the core as a lightweight
replacement 
for base.

-max (CORION on CPAN)

[1] http://corion.
net/perl-dev/parent.html
[2] http://
corion.net/perl-dev/parent-2.13.tar.gz

* Max Maischein <corioncorion.net>
[2007-08-01T16:20:28]
> b) Classes can be inherited from via other modules by
using an array 
> reference [ $class => $module ]:
> 
>   use parent [ Tie::StdHash => Tie::Hash ];

This is *really* confusing.  "So, the parent of
Tie::StdHash is Tie::Hash?"
Or... a dozen other things that could mean?  This looks much
clearer:

  use parent 'Tie::StdHash' => { defined_in =>
'Tie::Hash' };

...or something.

-- 
rjbs

Max Maischein wrote:
> Hello Porters,
> 
> I've followed the p5p discussion on base.pm and after
discussions with
> Anno and Bart wrote parent.pm ([1], [2]). So far I
haven't uploaded it
> onto CPAN as it's an all-lowercase module name, and I'm
waiting for the
> "go" from p5p.

Excellent, now I don't have to write it.  As far as I'm
concerned go right
ahead and upload it.


> The feature set is as follows:
> 
> a) Module loading is done via
> 
>   require $file
> 
> b) Classes can be inherited from via other modules by
using an array
> reference [ $class => $module ]:
> 
>   use parent [ Tie::StdHash => Tie::Hash ];

That's a good idea, except you have to quote the RHS as : is
not a word character.

	use parent [ "Tie::StdHash" => Tie::Hash ];

Also Tie::StdHash should just be moved into its own file to
avoid that.  Its
really the only widespread example of it I can think of.


> c) Classes can be inherited from without loading a file
by passing a
> false value as the module:
> 
>   use parent [ My::InnerClass => undef ];

Giving undef meaning other than 'error' makes my feet itch. 
Nothing more
concrete than that.


Further comments...

        next if $inheritor->isa($base);

I'm not so sure this is a good idea, skipping if it already
inherits.
Consider the following...

	package Top;
	sub foo 


	package Middle1;
	use parent [Top => undef];

	package Middle2;
	use parent [Top => undef];


	package Child;
	parent->import([Middle1 => undef, Top => undef]);

	print Child::ISA;

You only get Middle1 because Middle1 isa Top and by the time
it gets around to
inheriting from Top it already isa Middle1.  While this sort
of nasty, nasty
diamond inheritance is Evil and Wrong people do it and we
don't want to be
Surprising.

Other problems with that approach include...

	package Foo;

	use parent qw(This);
	use parent qw(That);
	use parent qw(This);

Which should produce Foo::ISA = qw(This That This) with
"This" being looked
at first, but actually produces Foo::ISA = qw(This That)
with "That" coming
first.  Subtle, and bad practice, but surprising.

I'd just drop the attempt to not put duplicates in ISA
entirely.  Allow the
user to blow their foot off if they like.  "use
parent" does not have a global
effect like "use lib" and so there is little
danger of unintentional
duplication from disparate parts of the code.  It was
probably intentional.


The other comment is that since ISA is altered before the
parent is required
there is the potential to leave a module with a dangling
inheritor should that
load fail and the error gets trapped.  Inherit after
requiring.


A further nit:

        next unless $module;

should be

        next unless defined $module;

because some joker is going to write "0.pm"



Finally, what review would be complete without a stylistic
nit?

        (my $filename = $module) =~ s!::|'!/!g;

! and | are really hard to distinguish.  Might I suggest
braces?

        (my $filename = $module) =~ s{::|'}{/}g;

Nice catch on the ' package separator.


Version 2.13?  I guess following on after base.pm?  Why
don't you make the
first few releases 0.0x to indicate how new it is and then
jump to 2.x?

Ricardo SIGNES wrote:
> * Max Maischein <corioncorion.net>
[2007-08-01T16:20:28]
>> b) Classes can be inherited from via other modules
by using an array 
>> reference [ $class => $module ]:
>>
>>   use parent [ Tie::StdHash => Tie::Hash ];
> 
> This is *really* confusing.  "So, the parent of
Tie::StdHash is Tie::Hash?"
> Or... a dozen other things that could mean?  This looks
much clearer:
> 
>   use parent 'Tie::StdHash' => { defined_in =>
'Tie::Hash' };
> 
> ...or something.

If nothing else it opens up a broader extension mechanism
for the future.

[ack, I didn't send this to the list, I hope this doesn't
mess up threading]

On 8/1/07, Brandon Black <blblackgmail.com> wrote:
> On 8/1/07, Max Maischein <corioncorion.net> wrote:
> > Hello Brandon,
> >
> >  > but I'd still like to see namespace
> > > detection for modules specified without an
arrayref which might not
> > > have a corresponding file at all,
> > ETOOMUCHMAGIC
> >
>
> This kind of magic is already in base.pm, and it's
really useful.
> It's let you do things like:
>
> package BaseClassInMyOwnFile
> # ...
> package Foo;
> use parent 'BaseClassInMyOwnFile;
>
> It also allows one to just do:
> use Tie::Hash;
> use parent 'Tie::StdHash';
>
> too.
>
> >  > ISA assignment at the end,
> > This would give the MRO a speed boost? Assiging at
the end would be no
> > problem, yes.
>
> It gives MRO a speed boost (but that only matters for
really complex
> hierarchies, more about it below), and it means that if
a require
> fails along the way, the original ISA is
left unchanged, instead of
> half-modified.
>
> >
> >  > and no
> > > explicit ".pm" (as this isn't
necessary and isn't compatible with .pmc
> > > files, and not very future-proof).
> > You mean I should change
> >
> >      require $module;
> >
> > to
> >
> >      eval "require $class"
> >
> > ? This would respect .pmc files then, and anything
that Perl can load,
> > at the "price" of having to reraise the
error:
> >
> >      die $ if $;
> >
>
> Yes, that 
>
> >  > I'd also prefer it not do the
> > > ->isa() check.  It doesn't save you
anything significant on older
> > > perls, it *really* has no useful effect on
5.9.5+, it might actually
> > > break future MROs that work differently that
dfs and c3, and doing
> > > that sort of "loop over (isa check,
require, push)" thing doesn't work
> > > with assigning all of the bases at the end.
> > I'm not sure what you're arguing for here... The
->isa() is a check that
> > came in from base.pm and I guess it's based on the
idea that we don't
> > want/need to inherit from something that we're
already inheriting from.
> > Is it that the new MRO(s) change the semantics in
ways that there
> > actually will be use cases for re-inheriting?  I
only accepted the
> >
> >      [ class => module ]
> >
> > case because the case of [ Tie::StdHash =>
Tie::Hash ] actually occurs
> > within Perl (and even the core).
> >
> > "loop over (isa, require, push)" works
as is now, and "loop over (isa,
> > require); push" would also work - "loop
over (require); push" should
> > also work, and with differing MROs too. I just
don't understand the
> > reasons why one is better than the other - can you
point me towards the
> > relevant (p5p?) messages?
> >
>
> The ->isa() check in base.pm, as far as I can tell,
is a performance
> micro-optimization.  In practice if everything else is
working
> correctly, eliding a parent that the class already
"isa" shouldn't
> change any behavior, and it cuts one more classname out
of those to
> search when finding methods in some cases.  Due to
caching and the
> simplicity of the 5.8.x equivalent of the mro code, the
optimization
> is tiny and hardly worth it.
>
> In 5.9.5+, the mro code actually explicitly eliminates
duplicate bases
> internally.  Inside of the mro code, your tree of
parent classes is
> reduced to a single linear list with no dupes.  This
makes the
> optimization totally useless on 5.9.5+.
>
> Further, it really is up to the mro algorithm in use
whether and when
> duplicated base classes can be elided.  It just so
happens that both
> dfs and c3 are unaffected by base.pm's optimization,
but there is not
> the facility for future versions of perl to have other
new mro
> algorithms in the core, some of which might not like
this at all.
>
> Finally, eliminating the ->isa() check is necessary
to take the
> push(ISA, $thing) out of the require loop.  This allows
the parent.pm
> code to loop on the "require"-related stuff,
but then just do a single
> assignment to ISA at the end of import().  Setting the
ISA
in a
> single assignment statement has performance benefits on
5.9.5+ over
> several pushes to ISA, and even over a single large push to
ISA
> (which is the same as several small ones internally). 
This is because
> the MRO algorithm is invoked to linearize the
inheritance tree every
> time an ISA is modified, but it has a performance
hack added such
> that adding several classes in a single assignment
statement only
> counts as one change.  There was no sane way to make
such an
> optimization for the push case, even when pushing them
all in one push
> statement.
>
> We can still have push behavior if the community thinks
it's a better
> idea, by doing the final assignment as "CallingClass::ISA =
> (CallingClass::ISA, new_bases)".  My
personal preference would be to
> move to explicit assignment, as in "CallingClass::ISA = new_bases".
> The fallout of getting rid of push behavior is that:
>
> use parent 'Foo';
> use parent 'Bar';
>
> results in just "Bar" as a parent, replacing
the previous "Foo".  I
> could be convinced that push is still the right
behavior to have, I'm
> not really strong on this point.
>
> -- Brandon
>

Brandon Black wrote:
>> Setting the ISA in a
>> single assignment statement has performance
benefits on 5.9.5+ over
>> several pushes to ISA, and even over a single
large push to ISA
>> (which is the same as several small ones
internally).

Orthogonal to this discussion... shouldn't that be optimized
internally?


>> We can still have push behavior if the community
thinks it's a better
>> idea, by doing the final assignment as "CallingClass::ISA =
>> (CallingClass::ISA, new_bases)".  My
personal preference would be to
>> move to explicit assignment, as in "CallingClass::ISA = new_bases".
>> The fallout of getting rid of push behavior is
that:
>>
>> use parent 'Foo';
>> use parent 'Bar';
>>
>> results in just "Bar" as a parent,
replacing the previous "Foo".  I
>> could be convinced that push is still the right
behavior to have, I'm
>> not really strong on this point.

I can't see a user wanting to replace the parent they just
inherited from.
There's plenty of ways to do something that wacky already if
you really want
to.  I can see the user spanning multiple inheritance across
multiple lines
and then being very surprised when it doesn't work.

I can also see someone trying something clever like this:

  BEGIN {
      ...do some clevar ISA manipulation...
  }

  use parent 'Simple::Class';

HEY!  Where'd my ISA go?!

On 8/1/07, Michael G Schwern <schwernpobox.com> wrote:
> Brandon Black wrote:
> >> Setting the ISA in a
> >> single assignment statement has performance
benefits on 5.9.5+ over
> >> several pushes to ISA, and even over a single
large push to ISA
> >> (which is the same as several small ones
internally).
>
> Orthogonal to this discussion... shouldn't that be
optimized internally?
>

I tried, you're welcome to take a stab at it too.  It
detects and
reacts to ISA changes via "magic", which means it has
to react every
time ISA changes.  Originally, any of these three
cases...

push(Foo::ISA, 'A'); push(Foo::ISA, 'B'); push(Foo::ISA,
'C');
push(Foo::ISA, 'A', 'B', 'C');
Foo::ISA = qw/A B C/;

... resulted in multiple pointless recalculations of the
mro, as the
way that "magic" works internally makes each value
change a separate
magic trigger.

Obviously the first case can't be optimized, we don't know
what might
occur between those separate statements.  I was able to
optimize the
third case, assignment of a list, by using the DELAYMAGIC,
but not the
second case, a push of several values at once.

-- Brandon

On Aug 01 2007, Brandon Black wrote:
> On 8/1/07, Michael G Schwern <schwernpobox.com> wrote:
> > Brandon Black wrote:
> > >> Setting the ISA in a
> > >> single assignment statement has
performance benefits on 5.9.5+ over
> > >> several pushes to ISA, and even over a single
large push to ISA
> > >> (which is the same as several small ones
internally).
> >
> > Orthogonal to this discussion... shouldn't that be
optimized internally?
> >
> 
> I tried, you're welcome to take a stab at it too.  It
detects and
> reacts to ISA changes via "magic", which
means it has to react every
> time ISA changes.  Originally, any of these three
cases...
> 
> push(Foo::ISA, 'A'); push(Foo::ISA, 'B'); push(Foo::ISA,
'C');
> push(Foo::ISA, 'A', 'B', 'C');
> Foo::ISA = qw/A B C/;
> 
> ... resulted in multiple pointless recalculations of
the mro, as the
> way that "magic" works internally makes each
value change a separate
> magic trigger.
> 
> Obviously the first case can't be optimized, we don't
know what might
> occur between those separate statements.  I was able to
optimize the
> third case, assignment of a list, by using the
DELAYMAGIC, but not the
> second case, a push of several values at once.

Try the attached patch.  It also uses PL_delaymagic but I
added a flag
(DM_ARRAY) for it instead of another global.  I also
reworked the aassign
to use this flag and removed the global you added
(PL_delayedisa).

It should be faster now to use push than assignment but I'll
leave the
mro doc updates to you.

-- 
Rick Delaney
rickbort.ca

On 8/3/07, Rick Delaney <rickbort.ca> wrote:
> On Aug 01 2007, Brandon Black wrote:
> > On 8/1/07, Michael G Schwern <schwernpobox.com> wrote:
> > > Orthogonal to this discussion... shouldn't
that be optimized internally?
> > >
> > I tried, you're welcome to take a stab at it too. 
It detects and
> > reacts to ISA changes via "magic", which
means it has to react every
> > time ISA changes.  Originally, any of these
three cases...
> >
> > push(Foo::ISA, 'A'); push(Foo::ISA,
'B'); push(Foo::ISA, 'C');
> > push(Foo::ISA, 'A', 'B', 'C');
> > Foo::ISA = qw/A B C/;
> >
> > ... resulted in multiple pointless recalculations
of the mro, as the
> > way that "magic" works internally makes
each value change a separate
> > magic trigger.
> >
> > Obviously the first case can't be optimized, we
don't know what might
> > occur between those separate statements.  I was
able to optimize the
> > third case, assignment of a list, by using the
DELAYMAGIC, but not the
> > second case, a push of several values at once.
>
> Try the attached patch.  It also uses PL_delaymagic but
I added a flag
> (DM_ARRAY) for it instead of another global.  I also
reworked the aassign
> to use this flag and removed the global you added
(PL_delayedisa).
>
> It should be faster now to use push than assignment but
I'll leave the
> mro doc updates to you.
>

I'm just now starting to look at it, but if it works, this
is great,
thanks   One thing
that caught my eye right off the bat in the
patch, is that it might not be safe on exceptions (such as
recursive
inheritance).  mro_isa_changed_in() can throw exceptions,
and
therefore mg_set on an ISA can as well, which is why I had
sometimes
contorted the code a bit to make sure that stuff happened at
the very
end of whatever was going on, after all housekeeping was
done, to
avoid leaks and/or breakage.  Soon as I get this built I'll
run some
tests and see what happens.

-- Brandon

On Aug 03 2007, Brandon Black wrote:
> I'm just now starting to look at it, but if it works,
this is great,
> thanks   One thing
that caught my eye right off the bat in the
> patch, is that it might not be safe on exceptions (such
as recursive
> inheritance).  mro_isa_changed_in() can throw
exceptions, and
> therefore mg_set on an ISA can as well, which is
why I had sometimes
> contorted the code a bit to make sure that stuff
happened at the very
> end of whatever was going on, after all housekeeping
was done, to
> avoid leaks and/or breakage.  Soon as I get this built
I'll run some
> tests and see what happens.

If you do find any problems, please code them up as
regression tests
before fixing the code or someone like me will probably just
break it
again.

-- 
Rick Delaney
rickbort.ca