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.wth@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.franck@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.wth@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.wth@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.fuchs@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 winterhalter@cr.openjdk.java.net: )

best regards and welcome!

-- daniel

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.franck@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.franck@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.wth@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.franck@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.wth@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.wth@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.fuchs@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 winterhalter@cr.openjdk.java.net: ) > > best regards and welcome! > > -- daniel >

I just deleted and recreated the file locally to avoid this now. No idea why the webrev command picked it up as a file change. I fixed the patch here: http://cr.openjdk.java.net/~winterhalter/8202473/webrev.02/ As for the possible refactoring: I had investigated it but in the factories for wildcard and parameterized types, the parameters are still provided with different arguments. Am Sa., 6. Juni 2020 um 11:44 Uhr schrieb Joel Borggrén-Franck < joel.borggren.franck@gmail.com>:

Hi Rafael,

Let's try to get this into 15,

http://cr.openjdk.java.net/~winterhalter/8202473/webrev.00/test/jdk/java/lan... 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.wth@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.franck@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.franck@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.wth@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.franck@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.wth@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.wth@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.fuchs@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 winterhalter@cr.openjdk.java.net: ) >>> >>> best regards and welcome! >>> >>> -- daniel >>> >>