[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: JDK 9 RFR of 8175335: Improve handling of module types in javax.lang.model.util.Types
From:       Alex Buckley <alex.buckley () oracle ! com>
Date:       2017-02-22 19:00:39
Message-ID: 58ADDFD7.7090501 () oracle ! com
[Download RAW message or body]

All looks good.

(Trivial: TypeMirror spec says "and to the keyword {@code void}" -- the 
"to" is unnecessary.)

Alex

On 2/22/2017 9:48 AM, joe darcy wrote:
> Hi Alex,
>
> Thanks for the careful reading.
>
> Revised webrev uploaded as
>
>           http://cr.openjdk.java.net/~darcy/8175335.1/
>
> Details comments below.
>
>
> On 2/21/2017 6:35 PM, Alex Buckley wrote:
>> On 2/21/2017 5:37 PM, joe darcy wrote:
>>> Please review the small spec and implementation changes for
>>>
>>>      8175335: Improve handling of module types in
>>> javax.lang.model.util.Types
>>>      http://cr.openjdk.java.net/~darcy/8175335.0/
>>>
>>> which treats pseudo-types for modules similarly to the pseudo-types for
>>> packages.
>>
>> Three specific comments:
>>
>> - Types::erasure(TypeMirror) is one of the family. It should throw IAE
>> on a module type as well as a package type.
>
> Diff old vs new changes:
>
> 152c152
> <      * @throws IllegalArgumentException if given a package type
> ---
>  >      * @throws IllegalArgumentException if given a type for a package
> or module
>
> (The implementation class was already updated to the new spec in the
> original webrev.)
>
>>
>> - Types::getNoType(TypeKind) could say "To obtain a pseudo-type for
>> packages or modules, call asType() on the result of
>> Elements.getPackageElement(CharSequence) or
>> Elements.getModuleElement(CharSequence)."
>
> Diff old vs new changes:
>
> 209,211c209,216
> <      * For packages, use
> <      * {@link Elements#getPackageElement(CharSequence)}{@code .asType()}
> <      * instead.
> ---
>  >      *
>  >      * <p>To get the pseudo-type corresponding to a package or module,
>  >      * call {@code asType()} on the element modeling the {@linkplain
>  >      * PackageElement package} or {@linkplain ModuleElement
>  >      * module}. Names can be converted to elements for packages or
>  >      * modules using {@link Elements#getPackageElement(CharSequence)}
>  >      * or {@link Elements#getModuleElement(CharSequence)},
>  >      * respectively.
>
>
>>
>> - TypeMirror's spec fails to mention module types in "pseudo-types
>> corresponding to packages and to the keyword void."
>
> Patched as follows:
>
> ---
> a/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
> Thu Feb 16 14:47:39 2017 -0800
> +++
> b/src/java.compiler/share/classes/javax/lang/model/type/TypeMirror.java
> Tue Feb 21 23:47:00 2017 -0800
> @@ -36,7 +36,7 @@
>    * array types, type variables, and the null type.
>    * Also represented are wildcard type arguments,
>    * the signature and return types of executables,
> - * and pseudo-types corresponding to packages and to the keyword {@code
> void}.
> + * and pseudo-types corresponding to packages, modules, and to the
> keyword {@code void}.
>    *
>    * <p> Types should be compared using the utility methods in {@link
>    * Types}.  There is no guarantee that any particular type will always
>
>
>>
>> A general comment:
>>
>> The methods in Types that throw IAE could be clarified. Where they now
>> say "if given a type for an executable, package, or module", they
>> really mean "if given a mirror that does not represent a suitable type
>> for the operation". The Types spec could say "Utility methods for
>> operating on types. Most methods operate on primitive types, reference
>> types (including array types and the null type), intersection types,
>> and the pseudo-type 'void'. Executable types and the pseudo-types for
>> packages and modules are generally out of scope for these methods."
>
> The general comment is valid, but I'd prefer to keep this bug about
> getting modules-as-types consistent with packages-as-types and leave the
> broader clarification you suggest to a future release. I've filed
> JDK-8175386: "Clarify exception behavior of Types utility methods" to
> track that work.
>
> Cheers,
>
> -Joe
>
[prev in list] [next in list] [prev in thread] [next in thread]