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