----- Mail original ----- > De: "Stuart Marks" <stuart.ma...@oracle.com> > À: "Remi Forax" <fo...@univ-mlv.fr> > Cc: "Martin Buchholz" <marti...@google.com>, "Kiran Ravikumar" > <kiran.sidhartha.raviku...@oracle.com>, "core-libs-dev" > <core-libs-dev@openjdk.java.net> > Envoyé: Mardi 11 Février 2020 00:57:52 > Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on > exception
> Hi Remi, > > The bug report originally requested that a bunch of different exceptions > include > a cause. I don't think the cause should be added to all of them. The cases > that > Kiran is adding are ones where I thought that adding a cause does have value. > > If someone is using a ListIterator (or a plain Iterator for that matter) they > might have an incorrect model for what the index value is after a certain > operation (remove, for example), and they might get an NSEE unexpectedly. They > might reasonably wonder what the state of the iterator is that resulted in > that > exception. Without a cause, NSEE doesn't have that information. Chaining the > IOOBE will usually include the index that caused the problem, which I think is > useful in such circumstances. > I don't disagree with the fact that having the index may help, I disagree with the fact that chaining the IOOBE to the NSEE is the right way to expose that index, crafting an error message with the index is IMO a better idea because you have no idea if the exception thrown by all implementation of List.get() will provide this index (and only this index). > The iterators in AbstractList all keep track of indexes and call get() for > access to the appropriate element. I don't think they should do a bounds check > and throw an exception on that basis, because the bounds could change between > the time they're checked and the call to get(). Thus, the iterators would have > to catch the exception from get() even if the bounds are checked in advance, > making the bounds check redundant. It depends how List.get() is implemented, if there is a bound check before accessing the value in the implementation, you have the same issue, one turtle down :) > > s'marks Rémi > > On 2/5/20 2:05 PM, Remi Forax wrote: >> Stuart, Martin, Kiran, >> I think this "bug" should not be fixed because it's one of the cases where >> providing more information is actually bad from a user POV. >> >> The current code throws NoSuchElementException when the iterator reach the >> end >> so from the user POV, this is the right exception because of the right issue, >> so from the user POV there is no need to change the actual code. >> If we chain the exception, >> - it's less clear from a user POV >> - the user may think that there is an error in the AbstractList >> implementation >> but it's not the case, it's just that AbstractList iterators next method is >> implemented weirdly, it prefers to go out of bound instead of checking the >> bound. >> >> I'm okay with NoSuchElementException having a new constructor that takes a >> chained exception but i really think that in this specific case, it's a bad >> idea(TM) to use it to propagate an exception to the user that it should not >> care about. >> >> BTW, perhaps, those method next() should be re-written to test the bound >> instead >> of catching the IOOBE because i'm not sure this "optimization" make sense >> nowadays. >> >> regards, >> Rémi >> >> ----- Mail original ----- >>> De: "Kiran Ravikumar" <kiran.sidhartha.raviku...@oracle.com> >>> À: "core-libs-dev" <core-libs-dev@openjdk.java.net> >>> Envoyé: Mercredi 5 Février 2020 20:49:09 >>> Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on >>> exception >> >>> Thanks Stuart and Martin, >>> >>> >>> Here is an updated webrev with the changes. >>> >>> http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/ >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8161558 >>> >>> >>> Thanks, >>> >>> Kiran >>> >>> >>> On 15/01/2020 12:46, Martin Buchholz wrote: >>>> Hi Kiran, >>>> >>>> Looks good to me, but I always nitpick ... >>>> >>>> Stray semicolon? >>>> var iterator = list.listIterator(list.size());; // position at end >>>> >>>> I would have documented whitebox test assumptions: that nCopies >>>> iterators are implemented via AbstractList, and that >>>> AbstractList's list iterator inherits behavior from iterator. >>>> >>>> I probably would have added a plain iterator test, and might have >>>> refactored the code that tests the exception. >>>> >>>> >>>> On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar >>>> <kiran.sidhartha.raviku...@oracle.com >>>> <mailto:kiran.sidhartha.raviku...@oracle.com>> wrote: >>>> >>>> Hi Guys, >>>> >>>> >>>> Could someone please review my fix to add missing standard >>>> constructor >>>> overloads to NoSuchElementException class and update the AbstractList >>>> class to use them. >>>> >>>> >>>> A CSR was filed and approved. Along with the code change a new >>>> test is >>>> added to verify the behavior. >>>> >>>> >>>> Please find the webrev at - >>>> >>>> >>>> http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/ >>>> >>>> >>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8161558 >>>> >>>> >>>> Thanks, >>>> > >>> Kiran
