Am 08.05.2012 12:30, schrieb David Holmes:
On 8/05/2012 8:22 PM, David Holmes wrote:
<restricting to core-libs-dev>
Hi Ulf,
On 8/05/2012 8:02 PM, Ulf Zibis wrote:
Hi all,
I'm a little bit late, but I just have seen:
(1) some indentations in the patch are broken
<sigh> I missed a couple of indent-2 that should be indent-4.
And something went awry with indentation in ensureProtectedAccess.
Unfortunately the webrev didn't show this and it isn't code that was
functionally modified (I had a tab-to-space conversion issue, which is
how this got touched at all).
Did you also noticed it in test class ?
(2) following code snipped is repeated many times:
4 times.
+ ClassLoader cl = tclass.getClassLoader();
+ ClassLoader ccl = caller.getClassLoader();
+ if ((ccl != null) && (ccl != cl) &&
+ ((cl == null) || !isAncestor(cl, ccl))) {
+ sun.reflect.misc.ReflectUtil.checkPackageAccess(tclass);
+ }
Wouldn't it be better, to move it in a method, maybe in
sun.reflect.misc.ReflectUtil ?
It didn't seem quite worth the effort of factoring out.
The isAncestor method is also repeated but again was not considered a
worthwhile factorization.
Now that I'm writing that I'm having my doubts, but at the time it was
expedient. Looking forward this code will likely have to change again to
suit a modular world.
I was forgetting there's another major consideration here too, and that is that this code sync's
up with Doug Lea's JSR-166 CVS repository. The changes I made are independent of the JDK used, but
if I refactored things into ReflectUtil then the code would only be valid on a JDK with that
update - which means Doug would need to maintain a different version of this fix for older JDKs.
Class ReflectUtil was just a suggestion. as it will be loaded anyway.
Looking in detail maybe whole AtomicXyzFieldUpdaterImpl constructors could be
factored out.
Same for the code snippet:
if (obj == null || obj.getClass() != tclass || cclass != null)
fullCheck(obj);
-Ulf
