On Wed, 20 Mar 2024 03:25:39 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated the fix and the test for multiple annotations > > test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107: > >> 105: newClassBytes = classBytes; >> 106: fInst.retransformClasses(targetClass); >> 107: assertTrue(targetClass.getName() + " was not seen by >> transform()", seenClassBytes != null); > > Nit: I guess, the `targetClassName` was intended to be used in place of > `targetClass.getName()`. The `retransform()` method returns an array of bytes. There is also a comment about it at the line 101. But the result has never been checked (please, see lines: 116, 123, 129, 135). This creates some confusion and questions why the return value is needed. The parameter `null` at lines 123, 129, 135 is also confusing and need some explanation. As I read the code, the method `retransform()` is saving the parameter `classBytes` in the `newClassBytes` field. The field `newClassBytes` value is then returned by the `transform()` method. But the `ClassFileTransformer.html#transform` states this: `Returns: a well-formed class file buffer (the result of the transform), or null if no transform is performed`. So that, when the `null` is returned by the `transform()` then it means there was no real transform. Could you comment on this? Do I miss anything here? Please, see: [ClassFileTransformer.html#transform](https://docs.oracle.com/en/java/javase/21/docs/api/java.instrument/java/lang/instrument/ClassFileTransformer.html#transform(java.lang.Module,java.lang.ClassLoader,java.lang.String,java.lang.Class,java.security.ProtectionDomain,byte%5B%5D)) Also, the comment at line 122 is not needed at this form: // Ensure retransformation does not fail with ClassFormatError. Sorry, I was not clear when I've asked to add a comment at this point. I wanted a clarification about what does passing the `null` mean in terms of transformation. My understanding is that there is no real transformation when the `null` is returned but the `Instrumentation` mechanism is still working and can fail and we want to test/verify it. So, passing a Null-transformer is enough for our testing. Is it correct? Please, keep in mind that few people have some knowledge about the re-transformation details and would appreciate any clarification here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1531502998
