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.san...@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.redes...@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.
