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 agree completely. This should be the normal use case for the WeavingURLClassLoader. 


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

  http://bugs.eclipse.org/bugs/show_bug.cgi?id=31460
Again right, this does fit my use-case, but I changed the implementation in a way that should provide a solution for both. See my comments on the bug: 

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


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.
This is the way I implemented the second version. A method "addAspect" is now protected to let subclasses access it. Hope that is okay to let the weaver be part of the 1.1 release?!? 


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.
I like this idea. Right now, the WeavingURLClassLoader uses the classes BcelWorld and BcelWeaver (aside from some others, of course). So we could define an interface for them. These interfaces could be loaded via the normal classpath. Then the implementation could be loaded via this private static class loader you mentioned and the work should be done. :-) 

Best regards,
Martin







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


_______________________________________________
aspectj-dev mailing list
aspectj-dev@xxxxxxxxxxx
http://dev.eclipse.org/mailman/listinfo/aspectj-dev

Back to the top