On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti <rgiulie...@openjdk.org>
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
The two calls to isStatic could be reordered closer together to be a single if
(isstatic()) { ... } else {... }.
src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
line 126:
> 124: public static int ACC_METH_PARAM_TYPES = 404;
> 125: public static int ACC_METH_NON_ACCESSIBLE = 405;
> 126:
I'd rather see few (fewer, just one) kinds of events, with so many different
kinds of events there needs to be a convenient method to look for any kind of
serialization mis-declaration.
Perhaps a single event with flags for the different kinds of errors.
The event classes could be responsible for turning the flags back into useful
messages on display.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 157:
> 155: }
> 156:
> 157: private static class A implements Serializable {
The test classes should document the good or badness of the class either in the
class/field/method names or in comments. What's wrong with this XXX.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431846034
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431850091
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431856800
