RFR 8186961 Class.getFields() does not return fields of previously visited super interfaces/classes.

[mandy chung]

On 11/30/17 12:17 PM, Paul Sandoz wrote:
> Here is the updated webrev:
>
>    http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/
>
> I opted for the simple solution using a LinkedHashSet.

This fix looks good except a typo in the test:

   94  "Class %s does not have extracly one field: %d", c.getName(), nfs));

s/extracly/exactly/

I am wondering if these @run should run with both othervm and agentvm mode since
it currently depends on the jtreg command-line (I believe our test target uses
agentvm as the default).

> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.

I was tempted to come up with optimization too when first reading the 
patch but I am with you.  I like the resulting code simple and clear.   
The difference is an array list vs linked hash set which we should wait 
until this is really a performance issue.  FWIW getMethods 
implementation also creates a linked hash set if not cached.

Mandy
> Paul.
>
> [*] when there are two or more super interface with fields, or one or more super interfaces and a super classes with fields.
>
>
>> On 30 Nov 2017, at 08:41, Paul Sandoz <Paul.Sandoz at oracle.com> wrote:
>>
>> Hi Caes,
>>
>> As we discussed off list the post loop logic is easily missed.
>>
>> However, i found another obvious issue i missed with two classes (super/sub) extending from the same interface that declares a field. See updated test case in the webrev.
>>
>> I have an idea to retain Field[] arrays and then process ‘em all at the end of the method to produce the final array. That should hopefully make the logic clearer too.
>>
>> Paul.
>>
>>
>>> On 29 Nov 2017, at 16:00, Claes Redestad <claes.redestad at oracle.com> wrote:
>>>
>>> Hi Paul,
>>>
>>> it seems you'll effectively skip processing of the last interface of c in the new code - is this intentional?
>>>
>>> 3049         Field[] iFields = null;
>>> 3050         for (Class<?> c : getInterfaces()) {
>>> 3051             if (iFields != null && iFields.length > 0) {
>>> ...
>>> 3060             }
>>> 3061             iFields = c.privateGetPublicFields();
>>> 3062         }
>>>
>>> ifaces is unused:
>>>
>>> 3047         Class<?>[] ifaces = getInterfaces();
>>>
>>> Nit: You could probably simplify code by replacing List<Field> fields with the LinkedHashSet directly, removing the need to create it conditionally.
>>>
>>> /Claes
>>>
>>>
>>> On 2017-11-29 21:15, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> Please review:
>>>>
>>>>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186961-iface-static-fields-get/webrev/>
>>>>
>>>> There is a bug lurking, perhaps for a while, where diamond patterns for interface hierarchies can result in the incorrect reporting of fields when using reflection, see the test case.
>>>>
>>>> Since reflection data is cached i don’t see an advantage to retaining a set of of traversed interfaces, which is the root cause of the issue.
>>>>
>>>> The code is optimized for common cases and will for less common cases collect fields of interfaces into a temporary linked hash set (to preserve the order, perhaps not strictly necessary but i did not want to change that behaviour).
>>>>
>>>> Thanks,
>>>> Paul.