RFR: JEP 359-Records: reflection code

[Remi Forax]

----- Mail original -----
> De: "joe darcy" <joe.darcy at oracle.com>
> À: "Vicente Romero" <vicente.romero at oracle.com>, "Brian Goetz" <brian.goetz at oracle.com>, "amber-dev"
> <amber-dev at openjdk.java.net>, "compiler-dev" <compiler-dev at openjdk.java.net>
> Envoyé: Mercredi 30 Octobre 2019 02:24:38
> Objet: Re: RFR: JEP 359-Records: reflection code

> Hello,
> 
> A few comments on the 01 iteration:
> 
> For Class.isRecord, I suggest explicitly stating that java.lang.Record
> is *not* a record.
> 
> It would be reasonable for ElementType to make some reference to the
> special annotation-handling rules for records.
> 
> For the equals method operating on floating-point values,
> Double.compare/Float.compare comparing the result against 0 are likely
> to do what one wants more so than ==. I see the ObjectMethods
> implementation already does this.

yes !
otherwise we can not have an inline record because equals() will be incompatible with how == works currently on an inline type.

> 
> For ObjectMethods, note that void.class is regarded by core reflection
> as being a primitive class and should likely be treated specially.
> 
> It seems odd their isn't a better return type than Object for the
> bootstrap method. Should the invokedynamic and dynamic constant cases be
> handled by the same method?
> 
> I recommend some more descriptive parameter names:
> 
>     @param theClass     the class hosting the components =>
> 
>     @param hostClass
> 
> At least in isolation, I don't think it is clear how the lookup object
> is used.
> 
> Cheers,
> 
> -Joe

Rémi

> 
> On 10/27/2019 4:41 PM, Vicente Romero wrote:
>> Hi Brian,
>>
>> Thanks for your comments. I have published another review iteration
>> [1]. Some comments inlined below.
>>
>> On 10/25/19 5:04 PM, Brian Goetz wrote:
>>> Spec in Class.java:
>>>
>>>  - Various {@code RecordComponent} should be {@link RecordComponent}.
>>
>> we could do that but the current approach is consistent with what we
>> are doing in similar APIs like Class::getDeclaredFields()
>>
>>>  - Rather than say "if this class was declared as a record in the
>>> source code", instead say "is a record class", since record classes
>>> are a term defined by the JLS.
>> fixed
>>>  - For isRecord/getRecordComponents, should include "@jls 8.10" link
>> done
>>>
>>> RecordComponent.java:
>>>
>>>  - Add @jls 8.10 link
>>>  - "Returns the name of the component represented by this record
>>> component." -> "Returns the name of this record component." and
>>> similar in other methods.  Since RC models a record component, its
>>> clear enough what is meant.
>>
>> done, the javadoc was following the same approach as in
>> java.lang.reflect.Field
>>
>>>  - For getGenericSignature, there should be a link back to @jls or
>>> @jvms that describes the format of the signature string.
>> done, added @jvms 4.7.9.1 Signatures
>>
>>>  - Doesn't getAnnotation() need some spec, or an {@inheritDoc}?
>>
>> yep done
>>
>> Thanks,
>> Vicente
>>
>> [1]
> > http://cr.openjdk.java.net/~vromero/records.review/reflection/webrev.01/