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 >>>>>>> >>>>>>
