On Wed, 24 Sep 2025 23:35:16 GMT, Stuart Marks <[email protected]> wrote:
>> Tagir F. Valeev has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Add SequencedCollection specialization for immutable collections >> - Fix indentation > > 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! ------------- PR Comment: https://git.openjdk.org/jdk/pull/27406#issuecomment-3443221548
