On Thu, 21 Dec 2023 09:53:10 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:
>
> Corrected @Label of event and of field.
src/java.base/share/classes/java/io/ObjectStreamClass.java line 466:
> 464:
> 465: if (SerializationMisdeclarationEvent.enabled() && serializable) {
> 466: new
> SerializationMisdeclarationChecker(cl).checkMisdeclarations();
Is there any benefit to avoiding the allocation and passing the class through
the methods as an extra arg?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 149:
> 147: if (!(spf instanceof ObjectStreamField[])) {
> 148: commitEvent(SERIAL_PERSISTENT_FIELDS_NAME +
> 149: " must be an instance of ObjectStreamField[] to be
> effective");
Hmmm...
The terminology "to be effective" seems a bit indirect and may lead to some
head scratching.
In most cases it means it is ignored.
I might suggest to just state the condition that is required and drop the extra
phrase.
For example, "xxx should be an instance of ObjectStreamField[]".
It would result in shorter messages and if further explanation of the message
is needed it can more completely elaborate on the impact of the incorrect
declaration.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 165:
> 163: commitEvent("method " + m + " on an enum class is not
> effective");
> 164: } else if (cl.isRecord()) {
> 165: commitEvent("method " + m + " on an record class is not
> effective");
"an record" -> "a record"
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434524764
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434512690
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434513714
