Hi Rafael, Let's try to get this into 15,
http://cr.openjdk.java.net/~winterhalter/8202473/webrev.00/test/jdk/java/lang/annotation/typeAnnotations/TypeVariableBoundParameterIndex.java.udiff.html looks strange did you unintentionally remove the test for 8202469? The issues are orthogonal so I'd like to keep the cases you developed for the previous fix. I think the fix can lead to further refactorings since this was the last use of buildAnnotatedType where parameter 4 and 5 wasn't identical. This can be followed up later. cheers /Joel On Tue, Apr 7, 2020 at 8:52 PM Rafael Winterhalter <rafael....@gmail.com> wrote: > I created this webrev to also fix the second part that was previously > fixed as part of 8202469: > http://cr.openjdk.java.net/~winterhalter/8202473/webrev.00/ - this would > be the second part of the adjustment. > > This is reported in https://bugs.openjdk.java.net/browse/JDK-8202473 and > was closed as a duplicate. With this cleaner solution, the duplication does > however no longer apply. > > Am Mo., 6. Apr. 2020 um 14:01 Uhr schrieb Joel Borggrén-Franck < > joel.borggren.fra...@gmail.com>: > >> Many thanks to Vicente for sponsoring this. I'll start to look at the >> second part shortly. >> >> cheers >> /Joel >> >> On Mon, Mar 16, 2020 at 9:44 PM Joel Borggrén-Franck < >> joel.borggren.fra...@gmail.com> wrote: >> >>> Looks good to me! >>> >>> I'll see if I can find a sponsor for this. >>> >>> cheers >>> /Joel >>> >>> On Sat, Mar 7, 2020 at 2:15 AM Rafael Winterhalter <rafael....@gmail.com> >>> wrote: >>> >>>> I finally found the time to look at this again, sorry for the delay. >>>> >>>> Thank you for your comments. I had the chance to better look into the >>>> problem and simplify the code a bit more and also reduced the scope of the >>>> fix to a single problem. I also added test cases that should be exhaustive >>>> for all possible scenarios and adjusted the code comment. >>>> >>>> I uploaded the adjusted patch as a webrev: >>>> http://cr.openjdk.java.net/~winterhalter/8202469/webrev.01/ >>>> >>>> Thanks, Rafael >>>> >>>> Am So., 16. Feb. 2020 um 10:51 Uhr schrieb Joel Borggrén-Franck < >>>> joel.borggren.fra...@gmail.com>: >>>> >>>>> Hi Rafael, >>>>> >>>>> Thanks for reaching out and reminding me of this! >>>>> >>>>> I managed to find some time to look into 8202469 during the weekend. >>>>> By swapping place of the TVars in the test I managed to isolate this >>>>> without depending on 8202473, take a look at my hacky copy-paste update to >>>>> your test at http://cr.openjdk.java.net/~jfranck/8202469/ . >>>>> >>>>> The comment on lines 269-276 needs to be updated. Perhaps include a >>>>> table with the start index depending of the type? Also referencing JVMLS >>>>> 4.4 for the structure of the bound might be instructive for future >>>>> readers. >>>>> >>>>> My current understanding is that there are two problems with the code >>>>> on (the original) lines 277-287: >>>>> 1) Type Variables should have index 0 while getting index 1 due to >>>>> lines 279-280. >>>>> 2) Bounds that are instances of ParameterizedType always gets index 1 >>>>> but if the first bound represents a Class type the index should be 0. >>>>> >>>>> Does this make sense? >>>>> >>>>> Can you make the if-switches positive? I find my original code with >>>>> the negative tests hard to read and the update doesn't help. >>>>> >>>>> Also can you expand the test to cover the different kinds of bounds >>>>> mentioned in 4.4 and also test Type Variables on methods, I suspect javac >>>>> treats method (and constructor) tvars similarly but there have been bugs >>>>> ... >>>>> >>>>> TL;DR please update the webrev with: >>>>> >>>>> - Split out test and fix for 8202469 >>>>> - More test coverage of different kinds of bounds >>>>> - Test for method tvars >>>>> - See if you can rework the logic to have (mostly) positive tests in >>>>> if switch >>>>> >>>>> Thanks again for looking into this, I'll start looking into 8202473, I >>>>> think the fix for that one opens up a bigger rework of the code which is >>>>> why I want to separate the two. >>>>> >>>>> cheers >>>>> /Joel >>>>> >>>>> On Sun, Aug 4, 2019 at 10:12 PM Rafael Winterhalter < >>>>> rafael....@gmail.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> appologies for the delay, I only managed to get my patched up to >>>>>> webrev today (http://cr.openjdk.java.net/~winterhalter/). It's three >>>>>> patches in total now, for the third one I could not add a test case, it >>>>>> would require to compile an annotation property against an enumeration >>>>>> type >>>>>> and load it against another class where the enumeration was turned into >>>>>> an >>>>>> annotation. I struggled a bit with jtreg to make it work but I cannot see >>>>>> myself complete this in a meaningful amount of time. However, I hope that >>>>>> the patch and the error it solves are straightforward to see. >>>>>> >>>>>> I only submitted a patch for 8202469. 8202473 is solved by it. It's >>>>>> two bugs but they are a symptom of the same oversight. >>>>>> >>>>>> I hope this helps you to review it but let me know if you have any >>>>>> questions or if I should further adjust my patch. >>>>>> >>>>>> Best regards, Rafael >>>>>> >>>>>> Am Fr., 2. Aug. 2019 um 12:18 Uhr schrieb Rafael Winterhalter < >>>>>> rafael....@gmail.com>: >>>>>> >>>>>>> Thanks Daniel, >>>>>>> I did some work on Glassfish a bunch of years ago, I had no idea. >>>>>>> I will try to split up the changes (I fixed another bug in >>>>>>> annotation processing yesterday) and upload everything there. >>>>>>> Best regards, Rafael >>>>>>> >>>>>>> Am Fr., 2. Aug. 2019 um 11:59 Uhr schrieb Daniel Fuchs < >>>>>>> daniel.fu...@oracle.com>: >>>>>>> >>>>>>>> Hi Rafael, >>>>>>>> >>>>>>>> On 02/08/2019 09:00, Joel Borggrén-Franck wrote: >>>>>>>> > I can host webrevs for you on cr to make it easier for other >>>>>>>> reviewers, if >>>>>>>> > you also send me the patches or webrefs off-list to get around the >>>>>>>> > attachment stripping I can upload them to cr. >>>>>>>> >>>>>>>> I see that you are a JDK project author, so you already own a page >>>>>>>> on cr (http://cr.openjdk.java.net/~winterhalter/) where you can >>>>>>>> upload webrevs (e.g. using `scp` as in: >>>>>>>> $ scp -r webrev-NNNNN.01 winterhal...@cr.openjdk.java.net: ) >>>>>>>> >>>>>>>> best regards and welcome! >>>>>>>> >>>>>>>> -- daniel >>>>>>>> >>>>>>>
