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