hg: jdk8/tl/jdk: 7103570: AtomicIntegerFieldUpdater does not work when SecurityManager is installed (original) (raw)

David Holmes david.holmes at oracle.com
Tue May 8 21:13:26 UTC 2012

On 9/05/2012 1:03 AM, Ulf Zibis wrote:

Am 08.05.2012 12:30, schrieb David Holmes:

On 8/05/2012 8:22 PM, David Holmes wrote:

On 8/05/2012 8:02 PM, Ulf Zibis wrote:

I'm a little bit late, but I just have seen: (1) some indentations in the patch are broken 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 ?

I don't see anything wrong in the test class. Indent is 4. ??

(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.

Factored out to where? There's no shared superclass here, so where would it get factored to?

Same for the code snippet: if (obj == null || obj.getClass() != tclass || cclass != null) fullCheck(obj);

Not part of my changes.

David

-Ulf

More information about the core-libs-dev mailing list