RFR: JDK-8209865: Incorrect 'multiple elements' notes with Elements#getTypeElement and --release & JDK-8209058: Cannot find annotation method 'value()' in type 'Profile+Annotation'

[Jonathan Gibbons]

Just checking ... the name of the webrev is "Code Review for jdk10", the 
workspace path ends in "/jdk10"
and the patch is named "jdk10.patch". I presume that's leftovers, right, 
and that this is really for JDK 12?

-- Jon

On 09/20/2018 04:57 PM, Jonathan Gibbons wrote:
> 2 +1's inline.
>
> -- Jon
>
> On 09/20/2018 07:35 AM, Jan Lahoda wrote:
>> On 20.9.2018 02:21, Jonathan Gibbons wrote:
>>>
>>>
>>> On 09/19/2018 08:54 AM, Jan Lahoda wrote:
>>>> Hi,
>>>>
>>>> I'd like to ask for a review of two related --release related bugs:
>>>>
>>>> The first bug is:
>>>> https://bugs.openjdk.java.net/browse/JDK-8209865
>>>>
>>>> Incorrect 'multiple elements' notes with Elements#getTypeElement and
>>>> --release
>>>>
>>>> The issue here is that
>>>> Elements.getTypeElement("javax.annotation.processing.Generated")
>>>> returns null. The cause is the structure of ct.sym: to share classfile
>>>> used for --release 8 and --release 9, the --release 9 is implemented
>>>> in a way that every module gets a plain directory with all classfiles
>>>> (whether they belong to the given module or not) on its path, and when
>>>> compiling, javac picks the correct packages/classes from this
>>>> directory based on module exports. But for the "unbound" search using
>>>> getTypeElement, all modules are searched, ignoring module exports, so
>>>> the Generated class is found in multiple modules.
>>>>
>>>> The proposed patch drops this ct.sym structure, and creates a simple
>>>> module-path-like structure for --release 9+ data:
>>>> 9/java.base
>>>>  /java.compiler
>>>> ...
>>>>
>>>> 9A/java.base
>>>>   /java.compiler
>>>> ...
>>>>
>>>> The paths for a module M for --release N are all directories in the
>>>> form ".*N.*/M". These contain only classes that belong to the given
>>>> module.
>>>>
>>>> For --release <= 8, the original approach remains. As a consequence,
>>>> there are no shared classfiles between JDK 8 and JDK 9 data, but the
>>>> size increase seems acceptable compared to the much cleaner structure
>>>> of ct.sym (originally about 6.2MB with JDK 11 data, about 7.9MB with
>>>> this patch and JDK 11 data).
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8209865
>>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8209865/webrev.00/
>>>
>>> This email is just about the first of your two bugs.
>>>
>>> So, this works because the data files for the releases remain 
>>> unchanged,
>>> and all you're doing here is changing the way those text files are
>>> "compiled" into ct.sym at build time, right, and subsequently read by
>>> JDKPlatformProvider at user-compile-time?
>>
>> Yes.
>>
>>>
>>> Separately, we should really have an (internal) spec for the format of
>>> files like ct.cym.   Yes, you're changing code, but it is surprising
>>> that there are no comments or any other documentation being updated
>>> along with the code changes to modify the format of the file.
>>>
>>> I understand that you want to move away from the old way of handling
>>> modules by introducing a new layer of module name. How hard would 
>>> it  be
>>> to share the data with versions <=8 by ignoring the enclosing module?
>>
>> Not that hard, a patch doing that is here:
>> http://cr.openjdk.java.net/~jlahoda/8209865/webrev.01/
>>
>> A delta from webrev.00:
>> http://cr.openjdk.java.net/~jlahoda/8209865/webrev.delta.00.01/
>
> +1
>
>>
>> And a patch fixing the second bug in a different way that does allow 
>> sharing the classfiles is here:
>> http://cr.openjdk.java.net/~jlahoda/8209058/webrev.01/
>
> +1
>
>>
>> The main drawback is that the bootclasspath for --release <= 8 will 
>> be longer (as it needs to include "879/java.base", 
>> "879/java.desktop", etc.)
>
> I don't see that as a big drawback, since it is now conceptually the 
> same across all versions.
>
>>
>> Thanks for the comments,
>>     Jan
>
> -- Jon
>
>
>>
>>>
>>> -- Jon
>