On Fri, 24 Oct 2025 13:40:49 GMT, Tagir F. Valeev <[email protected]> wrote:

>> OK, List12::getFirst and List12::getLast seem fine.
>> 
>> But yeah... AbstractImmutableList::reversed and List12::reversed are indeed 
>> debatable. I wrestled over this for a while. I don't think they're 
>> incorrect, but I'm also having trouble convincing myself they're cases worth 
>> optimizing. There are also weird tradeoffs.
>> 
>> With no override, calling reversed() on a List12 returns a 
>> ReverseOrderListView(orig) and reversed() on that returns orig again. But 
>> with the override, calling reversed() gives a new List12, and calling 
>> reversed() on that gives another new List12. Nothing here is really 
>> terrible, but it's also not clear to me this is an improvement.
>> 
>> With AbstractImmutableList I got kind of tangled up in analyzing where this 
>> would be used. It seems to me the optimization would take effect for the 
>> zero-size nulls-disallowed case, the zero-size nulls-allowed case, and the 
>> size-one nulls-allowed case. (The size-one nulls-disallowed case is covered 
>> by List12, unless we don't add the override there....) I don't think this is 
>> incorrect for any case, but I'm also having a hard time seeing the 
>> improvement. It might save the allocation of a small object, or it might 
>> save an indirection. And ... that's about it?
>> 
>> The reversed() overrides hardly seem worthwhile. Maybe don't include them.
>> 
>> Regarding the tests, is it necessary to add new test files for EmptyList and 
>> SingletonList? Maybe it's possible to add cases to 
>> test/jdk/java/util/Collection/MOAT.java. I'm also a bit surprised to see a 
>> new provider added to the ListFactories test. But I haven't looked too 
>> closely. If it's necessary, then of course we should include it.
>
> @stuart-marks a gentle ping: could you please review after the latest 
> changes? Thank you in advance!

Hi @amaembo thanks for making the updates as I requested. They look good. I ran 
the changes through our internal testing system and the results are good too. 
Also, thanks for your patience!

-------------

PR Comment: https://git.openjdk.org/jdk/pull/27406#issuecomment-3471296861

Reply via email to