Hi,
as expected this is quite a bit easier to follow. Thanks!
As any heavy use of reflection is likely to hit cached data, then
heavily optimizing might be ill-advised here.
A simpler optimization might be to check if the class has any superclass
or interfaces whatsoever first, since if not then publicFields ==
privateGetDeclaredField(true). This might reduce number of
LinkedHashSets created for many trivial class hierarchies significantly
for only a nominal increase in code complexity, and actually reduce the
retained set a bit.
/Claes
On 2017-11-30 21:17, 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.
>> It’s possible to heavily optimize (avoiding the production of a linked hash set until required [*]) but the resulting code is harder to understand.
>> 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.