8384062: Add a new method List::removeAtIndex to avoid overload confusion#31064
8384062: Add a new method List::removeAtIndex to avoid overload confusion#31064minborg wants to merge 1 commit into
Conversation
|
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
db6d6d4 to
d5fa4ee
Compare
|
/csr |
|
@minborg has indicated that a compatibility and specification (CSR) request is needed for this pull request. @minborg please create a CSR request for issue JDK-8384062 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
Mailing list message from Remi Forax on core-libs-dev: Hello, For users, it is clear that removeAtIndex() should be used. For implementors of java.util.List, it is not clear how to guarantee that remove(index) and removeAtIndex() are always in sync, We do not want to be in a situation where remove(index) and removeAtIndex() have two different implementations. So I think it's premature to create a PR until we discuss what can be done/should be done to solve that problem. Should we changed the tooling (javac, IDEs?) so implementation drift are recognized and reported ? regards, ----- Original Message -----
|
I don't think it's possible or necessary to try to establish any such guarantee. There are many constraints that List subclasses must uphold that the language and API cannot guarantee. We could provide some documentation for subclassers so that they don't make mistakes, e.g.,
|
|
I sympathize with this change: I also definitely sympathize with Remi's concern: we have two identically-behaving methods where one is the most correct one to invoke while the other is the most correct one to override. That's not ideal! We'd be happy if API-clients pretty much forgot I don't think there's a fix for this (barring extremely weird ideas). We can do it anyway, but I think that adding this API would have been a "no-brainer" otherwise, and I'm not sure it's totally a "no-brainer" now. Naming:
|
In many ways this would be preferable, but I think @minborg did some searching and found that it would result in a bunch of conflicts.
Yeah. On the one hand, if we wanted to minimize the overall number of methods in the system, then using There are a couple of other methods in |
|
I'll admit I'm a bit ambivalent about adding this (not that it is in any way my decision), and being forced to use the name |
This PR proposes to add a new
defaultmethodList::removeAtIndex.There are two overloads of the method
List::remove, and if the list is of typeList<Integer>, the overload resolution could pick a surprising variant.Hence, it is better to add a separate method that removes an element based on its index.
It is proposed that the
E remove(int index)method is not@Deprecated. Instead, we add verbiage to promote the new method over the old one.Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31064/head:pull/31064$ git checkout pull/31064Update a local copy of the PR:
$ git checkout pull/31064$ git pull https://git.openjdk.org/jdk.git pull/31064/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31064View PR using the GUI difftool:
$ git pr show -t 31064Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31064.diff
Using Webrev
Link to Webrev Comment