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

[Jan Lahoda]

On 21.9.2018 02:01, Jonathan Gibbons wrote:
> 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?

Yes, it is for JDK 12, I only still have the checkout inside directory 
named "jdk10". Sorry for the confusion.

Thanks for the reviews!

Jan

>
> -- 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
>>
>