On Thu, 4 Jun 2026 13:11:59 GMT, Chen Liang <[email protected]> wrote:
>> src/java.base/share/classes/java/io/ObjectStreamClass.java line 1384:
>>
>>> 1382: }
>>> 1383: if (!field.getType().equals(params[i])) {
>>> 1384: return false;
>>
>> Suggestion:
>>
>> return false; // Type mismatch
>
> `i == null` is not straightforward for "name mismatch" case. I should make
> that comment more specific, like "Name not accounted by the factory" and
> "Name match, type mismatch"
See the updated comments in the latest push.
>> test/jdk/java/io/Serializable/valueObjects/SimpleValueGraphs.java line 367:
>>
>>> 365: var obj = new ValueExt();
>>> 366: var ex = Assertions.assertThrows(InvalidClassException.class,
>>> () -> serialize(obj));
>>> 367: Assertions.assertEquals("SimpleValueGraphs$ValueExt; cannot
>>> serialize value class", ex.getMessage());
>>
>> Shouldn't this error message say "externalize" rather than "serialize"
>> (based off of the test name)
>
> I don't think we need distinct serialize/externalize messages for
> Externalizable: this distinction was arbitrary from an earlier
> implementation. The true distinction is migrated wrappers versus all other
> non-writeReplace Serializable value classes.
In the latest push, I removed the dead misleading message (which the old test
tested against) from `desc.isExternalizable() && !desc.isProxy()` branch in
`ObjectOutputStream` and added an assert indicating where an exception should
have been through.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2326#discussion_r3357051379
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2326#discussion_r3357079866
