Hi Nicolas, On Mon, 2005-11-28 at 19:08 +0100, Nicolas Geoffray wrote: > Here's two files, InstrumentationImpl and VMInstrumentationImpl. > I put all native vm specific methods in VMInstrumentationImpl, but > if you think it's not necessary, it's ok with me to have all these methods > in InstrumentationImpl.
This looks pretty good. Thanks! Some comments and questions: InstrumentationImpl should implement Instrumentation (then a couple of methods need to be made public). I believe the Vector of transformaers is handled correctly even when manipulated by multiple threads. But I like using an ArrayList and Iterator and do explicit synchronization (but in this case that is clearly just a personal preference, so feel free to ignore if you disagree that it is clearer.) How atomic do we want applyRedefineClasses() to be? Instead of looping through all classes to redefine and calling applyRedefineClass() on each one, it might be a good idea to have an applyRedefineClasses(Object[]) in VMInstrumentationImpl to redefine them all at the same time after preparing them all. Also if something went wrong with the transformation of any of them we might want to have a cancelRedefineClass() to let the VM know we won't be applying any prepared classes. Why does the private callTransformers() implementation take a ProtectionDomain? Isn't that classBeingRedefined.getProtectionDomain()? About the calltransformers FIXME. What else can/should we do here? Normally we like to provide some default/noop implementation for the VM reference classes. Is it possible to provide the necessary hooks in ClassLoader/VMClassLoader to call the transformers just before defineClass() is called? If we do this wouldn't it be slightly easier to have all these classes in vm/reference/java/lang to make package access easier? An example how this is all supposed to work together when a VM implements this VM interface would be nice. Also to make clear whether the premain() method is an instance of static method, what command line extensions/flags are expected, etc. Cheers, Mark _______________________________________________ Classpath-patches mailing list [email protected] http://lists.gnu.org/mailman/listinfo/classpath-patches
