Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/19206#issuecomment-2117380960
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. Looks good to me, great job, thanks! - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19206#pullrequestreview-2062430122
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Fri, 17 May 2024 06:03:00 GMT, Adam Sotona wrote:
>> Some tests are not migrated to the ClassFile API in previous migrations.
>>
>> - Some are simple oversights that didn't remove usages of
>> com.sun.tools.classfile;
>> - The CallerSensitive ones used an old utility, replaced by CF API-based
>> new code;
>> - many in javac are because the files are compiled with older source
>> compatibility. Those patches are converted to have the source code stored in
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
>> `CompilerUtils.compile`;
>> - As described in the JBS issue, there are a few other tests not covered;
>> one is in #19193 while the others are blocked by CreateSymbols migration or
>> bugs.
>>
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:
>
>> 149:
>> 150: boolean needsCsm = false;
>> 151: for (var element : code) {
>
> Scanning the instructions is a bit different approach than in the original
> test.
Same as the other method name check, it was manually extracted ReferenceFinder
logic, we should retire ReferenceFinder with the old class file library.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604405594
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Fri, 17 May 2024 06:07:47 GMT, Adam Sotona wrote:
>> Some tests are not migrated to the ClassFile API in previous migrations.
>>
>> - Some are simple oversights that didn't remove usages of
>> com.sun.tools.classfile;
>> - The CallerSensitive ones used an old utility, replaced by CF API-based
>> new code;
>> - many in javac are because the files are compiled with older source
>> compatibility. Those patches are converted to have the source code stored in
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
>> `CompilerUtils.compile`;
>> - As described in the JBS issue, there are a few other tests not covered;
>> one is in #19193 while the others are blocked by CreateSymbols migration or
>> bugs.
>>
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line
> 125:
>
>> 123: && invoke.method() instanceof MethodRefEntry ref
>> 124: && ref.owner().name().equalsString(className)
>> 125: && ref.name().equalsString(methodName)) {
>
> Is this additional test necessary, I don't see it in the original test.
This was part of the now-removed ReferenceFinder logic; it scans cp as a fast
path and then does instruction scan for all methods to find relevant methods to
pass to visitor.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604402631
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. test/langtools/tools/javac/7166455/CheckACC_STRICTFlagOnclinitTest.java line 81: > 79: ToolBox toolBox = new ToolBox(); > 80: toolBox.writeJavaFiles(in, SOURCE); > 81: CompilerUtils.compile(in, out, "--release", "16"); Smart move - PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604399988
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote:
> Some tests are not migrated to the ClassFile API in previous migrations.
>
> - Some are simple oversights that didn't remove usages of
> com.sun.tools.classfile;
> - The CallerSensitive ones used an old utility, replaced by CF API-based new
> code;
> - many in javac are because the files are compiled with older source
> compatibility. Those patches are converted to have the source code stored in
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
> `CompilerUtils.compile`;
> - As described in the JBS issue, there are a few other tests not covered;
> one is in #19193 while the others are blocked by CreateSymbols migration or
> bugs.
>
> Testing: all modified tests pass.
test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line
125:
> 123: && invoke.method() instanceof MethodRefEntry ref
> 124: && ref.owner().name().equalsString(className)
> 125: && ref.name().equalsString(methodName)) {
Is this additional test necessary, I don't see it in the original test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604394473
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote:
> Some tests are not migrated to the ClassFile API in previous migrations.
>
> - Some are simple oversights that didn't remove usages of
> com.sun.tools.classfile;
> - The CallerSensitive ones used an old utility, replaced by CF API-based new
> code;
> - many in javac are because the files are compiled with older source
> compatibility. Those patches are converted to have the source code stored in
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
> `CompilerUtils.compile`;
> - As described in the JBS issue, there are a few other tests not covered;
> one is in #19193 while the others are blocked by CreateSymbols migration or
> bugs.
>
> Testing: all modified tests pass.
test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:
> 149:
> 150: boolean needsCsm = false;
> 151: for (var element : code) {
Scanning the instructions is a bit different approach than in the original test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604390936
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. javadoc test change looks OK. - PR Review: https://git.openjdk.org/jdk/pull/19206#pullrequestreview-2055992753
