Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [aspectj-dev] Some minor changes to the code? 
Hi -

Thank you very much for this weaving class loader.  I've only looked 
at it briefly, and I'm glad that it seems to do the right security 
things.  (But I haven't tried it.)

I commented in the bug that I believe a weaving class loader must 
require all aspects be defined before loading any (affected) classes,
and that it should weave any classes it loads -- i.e., s.t. like

   WeavingClassLoader(URL[] aspectpath, URL[] classpath) { ... }
   // no methods for adding URLs to aspectpath or injars

I realize that might not fit your use-case.  For more comments, see

  http://bugs.eclipse.org/bugs/show_bug.cgi?id=31460

It's assigned to Jim, so my comments are merely advisory.  Also,
my initial inclination would be to defer patch discussions to the 
bug itself, but think this list is a good place to hash out 
what any aspect class-loading invariants should be.

Wes

P.S. - As a part of rolling this out in AspectJ, we should document
as an implementation limitation that any classes loaded by a 
parent loader are not woven and make clear the difference between
the classes-to-be-woven (per the compiler, the -injars input) and
classes-not-to-be-woven (per the compiler, the -classpath input)
-- in a perfect world, the latter would be a null set.

Also, if the weaving class loader must weave any classes it
loads, then to handle the common case of a namespace defined (as
the compiler does) by (1) an (unwoven) classpath, (2) an aspectpath 
defining aspects, and (3) a (woven) "injar" entry, the user will have
to define two class loaders, a parent to load the (unwoven) 
classpath and a child to load the (woven) -injars input (from
its "classpath").  We can do this in a factory method:

  /** ...
    * @param unwovenClasspath {like compiler's -classpath input}
    * ...
    * @param wovenClasspath {like compiler's -injars input}
    */
  public static WeavingClassLoader makeWeavingClassLoader(
      URL[] UnwovenClasspath,
      URL[] aspectpath,
      URL[] wovenClasspath) { ... }

This returns the child WeavingClassLoader, not only for convenience
but also to hide any later change to load the unwovenClasspath
directly from the weaving class loader (which I think is incorrect,
but I could be convinced otherwise).  Having a factory suggests
the constructor be package-private to enforce it in the default case.

P.P.S. - I should also say that I might expect users to subclass the loader
and provide API's to add aspects or aspectpath URL's after construction
for testing and other purposes, but it seems wrong for the 
AspectJ project to recommend that path.  So I wouldn't work to 
prevent subclasses from doing so at this point, aside from warning them.

P.P.P.S - I agree with Jim that using an IMessageHandler is the right
way for *us* to pass information around, but in the long run we should
support a weaving class loader usable by clients with access only
to it and Java 2 classes (e.g., the factory could use a private 
static class loader to load the implementation).  That way, only
a small jar for the weaving class loader itself has to be on the
classpath.  We'd need this to avoid conflicts with other versions 
of libraries (e.g., if BCEL or some eclipse libraries were used by 
clients of the weaving class loader), which we could do so long 
as the libraries were not loaded by any common parent class loader.  
But I think that's doable with an adapter class, and I'd prefer to 
have IMessage-based API's at least so we can use our existing 
testing libraries for specifying and comparing messages.

Martin Lippert wrote:
> 
> Hi!
> 
> >>The test cases for the class loader aren't yet in a state for
> >>contribution. So
> >>in general I would contribute the classloader to the project. But today
> >>seems to
> >>be a bit too early. Sorry!
> >
> >
> > I'm looking forward to seeing your weaving class loader implementation.  We knew that the new bytecode weaving architecture should enable this functionality, but it will be very nice to see working code.  I'll make your small requested changes.  I might do some additional refactoring to avoid code duplication.
> 
> Okay, here we are. I added my implementation to the bug system (no
> 31460). The code needs the changes I submitted earlier (no 30794) to
> compile.
> 
> I haven't found the time to implement all the test cases right now. So I
> will add them if they are finished, okay?
> 
> >>A better version of the "weaveWithoutDump" method would be an
> >>> implementation
> >>> that provides information which aspects got woven into the class (or none
> >>> if the
> >>> class remains unchanged). That would allow me to announce this information
> >>> within the weaving class loader which would be pretty nice.
> >
> >
> > This sort of information should probably be passed using the MessageHandler object that handles all other messages.  This object already receives IMessage.WARNING and IMessage.ERROR messages that you'll need to decide how to handle in your weaver.  It would be straightforward to add some additional IMessage.MESSAGE messages that would indicate when aspects were being woven into particular classes.  If you look at Shadow.java I bet you could find some commented out println's that would be exactly what you wanted if they were turned into messages.
> 
> The class loader already contains the hook method to announce dependency
> changes. The implementation using the MessageHandler object is prepared
> but not yet included.
> But now I can use the class loader within my special project and
> reimplement the announcement functionality later to find the exact set
> of aspects that got woven into a particular class. :-)
> 
> Best regards,
> Martin
> 
> _______________________________________________
> aspectj-dev mailing list
> aspectj-dev@xxxxxxxxxxx
> http://dev.eclipse.org/mailman/listinfo/aspectj-dev

Back to the top