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. @liach Your change (at version 4880f3a375ff46a4be94d0fe483f8ec927f1933a) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/19206#issuecomment-2117386568
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
RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
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. - Commit messages: - Missing copyright years - 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API Changes: https://git.openjdk.org/jdk/pull/19206/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19206=00 Issue: https://bugs.openjdk.org/browse/JDK-8332109 Stats: 1539 lines in 19 files changed: 224 ins; 1098 del; 217 mod Patch: https://git.openjdk.org/jdk/pull/19206.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19206/head:pull/19206 PR: https://git.openjdk.org/jdk/pull/19206
