garydgregory commented on PR #539: URL: https://github.com/apache/commons-bcel/pull/539#issuecomment-6340231172
@nbauma109 Please comment on or address these the following. ## Copying **`RuntimeVisibleTypeAnnotations.copy(ConstantPool)` and `RuntimeInvisibleTypeAnnotations.copy(ConstantPool)` do not fulfill their documented contract.** Both methods promise a _deep_ copy, but return a shallow clone, ignore the supplied constant pool, and share the original annotation array and mutable `AnnotationEntry` objects. Changing shared contents through the copy can therefore change the original. Existing ordinary-annotation classes have a similar defect, but repeating it here also causes a regression: these attributes previously used `Unknown.copy(ConstantPool)`, which copies its bytes and installs the supplied pool. The new methods should copy mutable contents independently and bind their constant-pool references to the supplied pool. Those references are described in [JVMS §4.7.20](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.20); the deep-copy requirement comes from BCEL’s Javadoc. The current test compares strings using the original poo l, so it needs additional checks for independent mutation and a different pool. ## Visting **`DescendingVisitor.visitTypeAnnotations(TypeAnnotations)` leaves the new visitor support incomplete.** It visits the attribute but does not visit the contained `AnnotationEntry` objects, unlike `DescendingVisitor.visitAnnotation(Annotations)`. With the new fixture, there are five type-annotation attribute callbacks and zero annotation-entry callbacks, despite six annotations being present. Previously, these annotations were inside `Unknown` attributes and were not traversed either, so this is missing functionality rather than a regression. The new method should traverse the wrapped annotation entries, with tests checking their callbacks and visitor stack context. The contained annotation structure is defined in [JVMS 4.7.20](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.20); the traversal expectation comes from BCEL’s existing visitor behavior. ## TypeAnnotationEntry **The new `TypeAnnotationEntry` API accepts malformed input, and some new Javadoc is inaccurate.** The public constructor documents `typePath` as a pair of bytes per step, but accepts a one-byte array. `dump(DataOutputStream)` then writes a path length of zero followed by that extra byte, shifting the following annotation fields. Dumping succeeds, but reading the result throws `EOFException`. This affects invalid caller input to the new API; valid parsed entries round-tripped successfully. Validation should reject malformed arrays before writing. [JVMS 4.7.20.2](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.20.2) defines the two-byte path entries, and [4.7.20.1](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.20.1) defines the target-info layouts. ## Javadoc The new Javadoc also needs corrections: `getTypePathLength()` says an empty path identifies the “whole type,” but for nested types it identifies the outermost part where an annotation is allowed; `TARGET_CLASS_EXTENDS` omits interface `extends` clauses; and the new attribute class summaries omit expression types and type-parameter declarations. These cases are described in [JVMS 4.7.20](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.20). Could you address each point, or explain the intended BCEL contract if any of these behaviors is intentional? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
