RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v3]

[Jonathan Gibbons]

On Mon, 30 Nov 2020 14:27:11 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Adding support for record classes in the historical data for ct.sym. This includes a few changes not strictly needed for the change:
>> -updating and moving tests into test/langtools, so that it is easier to run them.
>> -fixing Record attribute reading in javac's ClassReader (used for tests, but seems like the proper thing to do anyway).
>> -fixing the -Xprint annotation processor to print record component annotations.
>> 
>> Changes to jdk.jdeps' classfile library are needed so that the ct.sym creation works.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing tests on Windows - normalizing line endings.

Looks mostly OK; some minor comments inline.

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTest.java line 110:

> 108:                                               "--add-exports", "jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
> 109:                                               "--add-exports", "jdk.javadoc/jdk.javadoc.internal.api=ALL-UNNAMED",
> 110:                                               "--add-exports", "jdk.javadoc/jdk.javadoc.internal.tool=ALL-UNNAMED",

I'm surprised CreateSymbolsTest needs access to javadoc internals; is it because you're adding all of toolbox in lines 93, 94? Would it be better to filter out the classes you don't want to compile, such as `JavadocTask` and `JavapTask` ?

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java line 54:

> 52: import java.util.Enumeration;
> 53: import java.util.jar.JarEntry;
> 54: import java.util.jar.JarFile;

weird order of imports

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java line 238:

> 236:                            "t.Visible",
> 237:                            "package t;\n\n" +
> 238: //                           "@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" + //XXX

Is this line still supposed to be here?

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java line 270:

> 268:                            "t.Visible",
> 269:                            "package t;\n\n" +
> 270: //                           "@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" + //XXX

Is this line still supposed to be here?

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java line 307:

> 305:     }
> 306: 
> 307: //    @Test XXX

Another XXX line

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java line 513:

> 511:                              public java.util.List<java.lang.String> l();
> 512:                            }
> 513:                            """,

I don't understand why the lines with `\n` in a text block

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTest.java line 69:

> 67:             System.err.println("Warning: cannot find CreateSymbols, skipping.");
> 68:             return ;
> 69:         }

I can believe this test is worth just a warning, because you're trying to reach into the `make` directory, but the subsequent checks for `CreateSymbolsTestImpl.java` and `toolbox.*` seem worthy of errors, not warnings. On the other hand, I accept that all of these checks are very unlikely to fail, so maybe it doesn't matter.

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTest.java line 30:

> 28:  * @modules java.compiler
> 29:  *          jdk.compiler
> 30:  *          jdk.javadoc

See related comments in code ... seems strange to need `jdk.javadoc`.

test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTest.java line 121:

> 119:         }
> 120: 
> 121:         URLClassLoader cl = new URLClassLoader(new URL[] {testClasses.toUri().toURL(), compileDir.toUri().toURL()});

line 132: the name of the local variable `createSymbols` seems more specific than the method might imply!

-------------

PR: https://git.openjdk.java.net/jdk/pull/1480