On Tue, Sep 22, 2009 at 08:43:08PM -0500, Jesse Luehrs wrote:
> On Tue, Sep 22, 2009 at 06:16:48PM -0700, Darren Duncan wrote:
> > 1. I looked for the implementation of is_class_loaded() and it seems the
> > only implementation is XS, in the file MOP.xs, with no Pure Perl version
> > existing, although the code comments with that XS says there is a Pure
> > Perl version in lib/Class/MOP.pm which one should study to better
> > understand the "grotty logic". Now I interpret this as that the code
> > comment is outdated and the Pure Perl version was removed sometime around
> > when it was decided to make a C compiler a mandatory requirement of using
> > CMOP. If I'm wrong, please point out the Pure Perl version, or else the
> > comment needs updating.
>
> Yeah, that comment is outdated, for the reasons you mentioned.
Fixed.
> > 2. From lib/Class/MOP.pm:
> >
> > sub _try_load_one_class {
> > my $class = shift;
> > return if is_class_loaded($class);
> > my $file = _class_to_pmfile($class);
> > return do {
> > local $@;
> > local $SIG{__DIE__};
> > eval { require($file) };
> > $@;
> > };
> > }
> >
> > One thing that I'm doing which CMOP doesn't seem to do is test after an
> > error-free eval that the successfully compiled module actually declares a
> > class of the requested name. For example, by adding another
> > is_class_loaded() call after the "eval" line iff $@ doesn't hold an
> > error. So the way CMOP is now, load_class() seems to return true if it
> > compiles the appropriately named file, and regardless of whether the
> > desired class was declared by it.
> >
> > That seems to be an error to me. Would you agree or disagree?
>
> Not sure about this one... it sounds reasonable, but don't know if other
> code might be relying on this?
>
> > 3. I seem to recall reading somewhere, maybe in the Try::Tiny docs, that
> > it is a common error to test $@ for truthiness rather than definedness.
> > CMOP is just testing for truthiness, which would I think fail if someone
> > throws an exception consisting of the number zero or the empty string.
> >
> > So that also seems to be an error in CMOP. Agree/disagree?
> >
> > P.S. Yes, I recognize my own code also has some of the same mistakes of
> > #3, and I'll fix them.
>
> Yeah, there are probably a bunch of places that we should be using
> Try::Tiny rather than eval... this does look to be one of them.
I've got a branch which converts all uses of block-eval in Class-MOP and
Moose to use Try::Tiny instead, that should fix this issue.
-doy
