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