On Thu, 4 Jun 2026 11:19:22 GMT, Viktor Klang <[email protected]> wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Message updated
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 2243:
>
>> 2241: * the exit of {@link #readObject(Class)}.
>> 2242: */
>> 2243: private Object readRecordOrValue(ObjectStreamClass desc, boolean
>> isRecord) throws IOException {
>
> My preference would be to introduce a readValue instead of passing around a
> boolean to indicate whether record or value.
Good point. The boolean is actually accessible as `desc.isRecord()`. I have now
renamed this class to `FactorySupport` and split the record and factory paths,
leaving the method handle building code in common.
> src/java.base/share/classes/java/io/ObjectStreamClass.java line 126:
>
>> 124: private boolean isRecord;
>> 125: /** true if represents a value class */
>> 126: private boolean isValueOrStrictInit;
>
> The comment reads one thing but the name of the field tells a slightly
> different story. If we need to differentiate "value or strict init" in the
> variable name, then it would make sense to explain in the comment.
Right. I renamed this to `requiresDeserializeFactory` to better convey the
intentions.
> src/java.base/share/classes/java/io/ObjectStreamClass.java line 848:
>
>> 846: }
>> 847:
>> 848: boolean isDeserializeConstructor() {
>
> This method name reads a bit odd. Is it "useDeserializeConstructor",
> "hasDeserializeConstructor", "isDeserializeConstructor" or would there exist
> a better name to reflect its purpose?
Renamed to `hasDeserializeConstructor`, pairs up with the new
`requiresDeserializeConstructor`.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2326#discussion_r3357066175
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2326#discussion_r3357057223
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2326#discussion_r3357069331
