Re: [PATCH] XSLoader for Storable
On Thu, Jun 30, 2005 at 12:41:03PM +0100, Paul Marquess wrote: > I've got a request to make the same change to Compress::Zlib. Has a > conclusion been reached on the definitive way to write this yet? Somebody who actually uses XS take over XSLoader from me, put a copy on CPAN and put an end to all the "if XSLoader else DynaLoader" nonsense. Here it is, all ready to go: http://svn.schwern.org/svn/CPAN/XSLoader/ -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern ROCKS FALL! EVERYONE DIES! http://www.somethingpositive.net/sp05032002.shtml
RE: [PATCH] XSLoader for Storable
From: Michael G Schwern [mailto:[EMAIL PROTECTED] > On Mon, Jun 27, 2005 at 09:01:15AM +0400, Alexey Tourbin wrote: > > > DynaLoader is documented as needing to be inherited from. You screw > with > > > that you flirt with breakage just to avoid writing one line of code. > > > > My patch has nothing to do with subclassing DynaLoader, which is still > > perfectly posslible. :) > > "local @ISA = qw(DynaLoader)" looks like subclassing to me. You have to > be > a subclass of DynaLoader in order to use bootstrap(). I've got a request to make the same change to Compress::Zlib. Has a conclusion been reached on the definitive way to write this yet? Paul ___ Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com
Re: [PATCH] XSLoader for Storable
On Mon, Jun 27, 2005 at 09:01:15AM +0400, Alexey Tourbin wrote: > > DynaLoader is documented as needing to be inherited from. You screw with > > that you flirt with breakage just to avoid writing one line of code. > > My patch has nothing to do with subclassing DynaLoader, which is still > perfectly posslible. :) "local @ISA = qw(DynaLoader)" looks like subclassing to me. You have to be a subclass of DynaLoader in order to use bootstrap(). -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern You are wicked and wrong to have broken inside and peeked at the implementation and then relied upon it. -- tchrist in <[EMAIL PROTECTED]>
Re: [PATCH] XSLoader for Storable
On Sun, Jun 26, 2005 at 09:38:47PM -0700, chromatic wrote: > > Please observe the dire warning layed out in my sig below. > > > > DynaLoader is documented as needing to be inherited from. You screw with > > that you flirt with breakage just to avoid writing one line of code. > > Most of its documentation suggests using a standard function call, not > to mention the usage error seven lines in. If it really needs > inheritance and a method call, it needs a documentation patch (and > hopefully one that avoids the blepharitic indirect object syntax). Yes, much of the DynaLoader documentation is confused. DynaLoader exports no functions yet the documentation suggests that it does. XSLoader gets around this madness by working largely in package DynaLoader. The use of indirect object syntax is definately to blame here. bootstrap() is a method, not a function. And then there's the OO / not-OO confusion. Some of DynaLoaders routines are functions (dl_find_symbol(), for example) and some are methods (dl_load_flags()). To add to the confusion the methods are often hidden as indirect object calls. Nice and inconsistent and confusing. Its like whoever wrote DynaLoader was trying to hide its OO nature. Given the general dislike of OO at the time of DynaLoader's inception I'm not surprised. I'd take a stab at fixing the docs but I'm not an XS author so I'm afraid I'd get things wrong. Fixing the documentation is one thing. Fixing the interface... ugg. Might do more harm than good to try that. Which is why I'm pushing getting XSLoader onto CPAN. 99% of the DynaLoader interface is not needed by 99% of XS module authors. -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern You are wicked and wrong to have broken inside and peeked at the implementation and then relied upon it. -- tchrist in <[EMAIL PROTECTED]>
Re: [PATCH] XSLoader for Storable
On Sun, Jun 26, 2005 at 09:03:00PM -0700, Michael G Schwern wrote: > On Sun, Jun 26, 2005 at 08:36:39PM -0700, Yitzchak Scott-Thoennes wrote: > > AIUI the only time it needs to be ISA DynaLoader is during the > > bootstrap call. > > Please observe the dire warning layed out in my sig below. > > DynaLoader is documented as needing to be inherited from. You screw with > that you flirt with breakage just to avoid writing one line of code. My patch has nothing to do with subclassing DynaLoader, which is still perfectly posslible. :) However, I need some time to consider your argument. > -- > Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern > You are wicked and wrong to have broken inside and peeked at the > implementation and then relied upon it. > -- tchrist in <[EMAIL PROTECTED]> pgpO2DGvKCjr3.pgp Description: PGP signature
Re: [PATCH] XSLoader for Storable
On Sun, 2005-06-26 at 21:03 -0700, Michael G Schwern wrote: > Please observe the dire warning layed out in my sig below. > > DynaLoader is documented as needing to be inherited from. You screw with > that you flirt with breakage just to avoid writing one line of code. Most of its documentation suggests using a standard function call, not to mention the usage error seven lines in. If it really needs inheritance and a method call, it needs a documentation patch (and hopefully one that avoids the blepharitic indirect object syntax). -- c
Re: [PATCH] XSLoader for Storable
On Sun, Jun 26, 2005 at 06:35:49PM -0700, Michael G Schwern wrote:
> On Mon, Jun 27, 2005 at 05:10:02AM +0400, Alexey Tourbin wrote:
> > +} or do {
> > +require DynaLoader;
> > +local @ISA = qw(DynaLoader);
> > +bootstrap Storable $VERSION;
> > +};
> > 1;
> > __END__
> > #
> > End of patch
> >
> > Since Storable is also on CPAN, the code is conditinal. "local @ISA"
> > is tricky, I think I've seen this elsewhere.
>
> That doesn't seem right. It means Storable only inherits from DynaLoader
> in the scope of that bootstrap call and I don't see why that's necessary.
> It wasn't necessary before.
>
> "push @ISA, qw(DynaLoader)" is safer and equivalent to what was happening
> before the patch.
I'm not sure. I've seen this in List::Util and I thought it was
"tricky" or even "smart".
After all, why being 'IS A' Dynaloader is essential for the package?
Because bootstrap() calls dl_load_flags() method which can be overridden
in the package (for e.g. "global loading", used in e.g. gtk2-perl.sf.net
modules). After bootstrap is done, the package no longer needs to be
'IS A' DynaLoader.
On the other hand, when the package does not override dl_load_flags(),
bootstrap will look into %Exporter:: first and only after this will
default to DynaLoader::dl_load_flags(). Looking into Exporter is
basically not required.
pgprv1Y8fDdsm.pgp
Description: PGP signature
Re: [PATCH] XSLoader for Storable
On Sun, Jun 26, 2005 at 08:36:56PM -0700, chromatic wrote: > Even easier (especially on the eyes): > > DynaLoader::bootstrap( 'Storable', $VERSION ); On Sun, Jun 26, 2005 at 08:36:39PM -0700, Yitzchak Scott-Thoennes wrote: > AIUI the only time it needs to be ISA DynaLoader is during the > bootstrap call. Please observe the dire warning layed out in my sig below. DynaLoader is documented as needing to be inherited from. You screw with that you flirt with breakage just to avoid writing one line of code. There is the odd and largely unused bootstrap_inherit() function which does the local @ISA thing. Its undocumented and the only thing which uses it is XSLoader [1] which does all sorts of incestuous things with DynaLoader so its techniques cannot be trusted. List::Util does the local @ISA thing. It, too, flirts with doom. It would be great if DynaLoader did not need to be inherited from, this would avoid all sorts of niggling little problems like accidental AutoLoader inherited. I can imagine that's the inspiration for local @ISA. But that's a separate issue from patching up Storable to use XSLoader. And anyway, isn't that what XSLoader was made for in the first place? Finally, the hacks in this patch are to avoid a dependency on XSLoader which is currently not available from CPAN. However, some time ago I released an alpha of XSLoader to CPAN: http://search.cpan.org/dist/XSLoader/ http://svn.schwern.org/svn/CPAN/XSLoader/ under the cavet that I cannot maintain it as I really don't understand it. Its there for anyone who wants to pick up the baton and run. The job is really just integrating changes from bleadperl, releasing a CPAN version and trying to keep it something like backwards compatible. Take it over and release a stable version and all this messing with DynaLoader goes away. [1] wince/makedist.pl parrots the function but does not appear to use it -- Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern You are wicked and wrong to have broken inside and peeked at the implementation and then relied upon it. -- tchrist in <[EMAIL PROTECTED]>
Re: [PATCH] XSLoader for Storable
On Sun, 2005-06-26 at 18:35 -0700, Michael G Schwern wrote: > That doesn't seem right. It means Storable only inherits from DynaLoader > in the scope of that bootstrap call and I don't see why that's necessary. > It wasn't necessary before. > > "push @ISA, qw(DynaLoader)" is safer and equivalent to what was happening > before the patch. Even easier (especially on the eyes): DynaLoader::bootstrap( 'Storable', $VERSION ); -- c
Re: [PATCH] XSLoader for Storable
On Sun, Jun 26, 2005 at 06:35:49PM -0700, Michael G Schwern wrote:
> On Mon, Jun 27, 2005 at 05:10:02AM +0400, Alexey Tourbin wrote:
> > +} or do {
> > +require DynaLoader;
> > +local @ISA = qw(DynaLoader);
> > +bootstrap Storable $VERSION;
> > +};
> > 1;
> > __END__
> > #
> > End of patch
> >
> > Since Storable is also on CPAN, the code is conditinal. "local @ISA"
> > is tricky, I think I've seen this elsewhere.
>
> That doesn't seem right. It means Storable only inherits from DynaLoader
> in the scope of that bootstrap call and I don't see why that's necessary.
> It wasn't necessary before.
>
> "push @ISA, qw(DynaLoader)" is safer and equivalent to what was happening
> before the patch.
AIUI the only time it needs to be ISA DynaLoader is during the
bootstrap call.
Re: [PATCH] XSLoader for Storable
On Mon, Jun 27, 2005 at 05:10:02AM +0400, Alexey Tourbin wrote:
> +} or do {
> +require DynaLoader;
> +local @ISA = qw(DynaLoader);
> +bootstrap Storable $VERSION;
> +};
> 1;
> __END__
> #
> End of patch
>
> Since Storable is also on CPAN, the code is conditinal. "local @ISA"
> is tricky, I think I've seen this elsewhere.
That doesn't seem right. It means Storable only inherits from DynaLoader
in the scope of that bootstrap call and I don't see why that's necessary.
It wasn't necessary before.
"push @ISA, qw(DynaLoader)" is safer and equivalent to what was happening
before the patch.
--
Michael G Schwern [EMAIL PROTECTED] http://www.pobox.com/~schwern
Just call me 'Moron Sugar'.
http://www.somethingpositive.net/sp05182002.shtml
