Re: RFR 7118066: Warnings in java.util.concurrent package

2011-12-05 Thread David Holmes
On 6/12/2011 12:14 PM, David Holmes wrote: On 6/12/2011 11:45 AM, Rémi Forax wrote: On 12/06/2011 02:12 AM, David Holmes wrote: Is the reason for constructs like this: HashEntry[] tab = (HashEntry[])new HashEntry[cap]; that we can't utilize diamond? Otherwise it would nicely redu

Re: RFR 7118066: Warnings in java.util.concurrent package

2011-12-06 Thread David Holmes
Thanks Doug and Chris. They were just nits so it is fine to proceed - thanks for changing the annotations though. David On 7/12/2011 11:33 AM, Doug Lea wrote: Thanks for all the comments. We changed to having the annotations on their own lines (even though it furthers the Java tendency of gr

Re: review of 7117249: java.util warnings patches from LJC/Mike Barker

2011-12-07 Thread David Holmes
On 7/12/2011 9:12 PM, Martijn Verburg wrote: Hi all, Question on contributions, I assume that as the patch is coming from Mike, his OCA covers the rest of us? These patches are simple enough that OCA is not necessary - any Participant can submit simple patches [1] David -- [1] http://o

Re: RFR 7118066: Warnings in java.util.concurrent package

2011-12-07 Thread David Holmes
ptyListIterator()); - testToArray(c); } -Chris. On 07/12/2011 06:04, David Holmes wrote: Thanks Doug and Chris. They were just nits so it is fine to proceed - thanks for changing the annotations though. David On 7/12/2011 11:33 AM, Doug Lea wrote: Thanks for all the comments. We changed to

Re: RFR 7118066: Warnings in java.util.concurrent package

2011-12-08 Thread David Holmes
On 8/12/2011 9:33 PM, Chris Hegarty wrote: On 08/12/2011 05:13, David Holmes wrote: I also think the SynchronousQueue has no business being used in this test. Though I confess I'm unclear what this test is for - is it for emptyIterator() or for "iterators of empty collections&q

Re: array and diamond

2011-12-11 Thread David Holmes
On 10/12/2011 8:56 AM, Rémi Forax wrote: Is there a reason why the diamond syntax can't be used with an array ? List[] list = new List<>[12]; It can in Java 7. That's the bug that we just discussed in regard to the java.util.concurrent code cleanup where I suggested to use: HashEntry[] new

Re: Code Review Request for Bug #7089443

2011-12-12 Thread David Holmes
It seems that 7089443 is fixed as a by-product of your changes Charles - provided getnameinfo does the right thing :) David -- On 13/12/2011 12:26 PM, Charles Lee wrote: On 12/13/2011 06:50 AM, Darryl Mocek wrote: Hello. Please review this patch to fix InetAddress.getLocalHost fails if th

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

2011-12-12 Thread David Holmes
Hi Fred, A couple of minor comments on the JDK side: HotSpotDiagnosticMXBean.java: Sorry if this is old ground but a query on the API design: is there a specific reason to use Lists rather than the arrays the VM will provide? HotSpotDiagnostic.java: 139 public List getDiagnosticCommand

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-12 Thread David Holmes
e target array will be copied to a new array that exactly fits the number of elements returned". David Holmes Here is a testcase: // import java.util.Map; import java.util.concurrent.Conc

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-13 Thread David Holmes
On 13/12/2011 8:16 PM, Ulf Zibis wrote: Am 13.12.2011 08:41, schrieb David Holmes: Hi Sean, On 13/12/2011 5:21 PM, Sean Chou wrote: When I was reading the code of AbstractCollection.toArray(T[] ), I found its behavior maybe different from the spec in multithread environment. The spec says &qu

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-13 Thread David Holmes
Map, as per the example, then perhaps ConcurrentHashMap should be where a change is considered. David - On Tue, Dec 13, 2011 at 3:41 PM, David Holmes mailto:david.hol...@oracle.com>> wrote: Hi Sean, On 13/12/2011 5:21 PM, Sean Chou wrote: When I wa

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

2011-12-13 Thread David Holmes
e in-lined below. Fred On 12/13/11 07:49 AM, David Holmes wrote: Hi Fred, A couple of minor comments on the JDK side: HotSpotDiagnosticMXBean.java: Sorry if this is old ground but a query on the API design: is there a specific reason to use Lists rather than the arrays the VM will provide? Lis

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-13 Thread David Holmes
Sean, Ulf, I retract all my previous comments - they were completely wrong. AbstractCollection.toArray _does_ address concurrent modification: "This implementation returns an array containing all the elements returned by this collection's iterator in the same order, stored in consecutive ele

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-14 Thread David Holmes
it may be possible to modify the behavior without causing > problems. > > And I think modifying ConcurrentHashMap is as dangerous as modifying > AbstractCollection > if people are relying on implementation, is this right? So it seems we can

Re: A behavior mismatch in AbstractCollection.toArray(T[] )

2011-12-14 Thread David Holmes
11 at 4:02 PM, David Holmes mailto:david.hol...@oracle.com>> wrote: On 14/12/2011 5:05 PM, Sean Chou wrote: Hi Mike, David, I reported this as a bug: http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7121314 <http://bugs.sun.com/bugdatabase

Re: 100218: BigInteger staticRandom field

2011-12-15 Thread David Holmes
ve for the lifetime of the thread. --- David Holmes

Re: Code Review Request for Bug #4802647

2011-12-19 Thread David Holmes
Brandon, I don't see the purpose of NewAbstractSet. It is identical to NewAbstractCollection. Otherwise my only concern is as you raised previously, updating the test to check existing subclasses causes new failures for ArrayList and CopyOnWriteArrayList. I'd suggest fixing ArrayList as part

Re: Code Review Request for Bug #4802647

2011-12-23 Thread David Holmes
Hi Brandon, This looks okay to me. Just to clarify for other readers, the change to CopyOnWriteArrayList doesn't use Objects.requireNonNull because this code has to sync up with Doug Lea's jsr166 repository which runs on multiple version of the platform (some without requireNonNull). Also C

Re: 100218: BigInteger staticRandom field

2012-01-03 Thread David Holmes
e is the bottleneck (isProbablePrime?), under what conditions - is it a microbenchmark or real code? Thanks, David Holmes

Re: 100218: BigInteger staticRandom field

2012-01-03 Thread David Holmes
On 4/01/2012 10:08 AM, David Holmes wrote: Hi Paul, For some reason this email, despite being dated Dec 14, only just appeared in my inbox on Jan 3! Oops! I missed the fact a copy of this email also turned up on Dec 15 and that I already replied to it. Comments still apply. Need to

Re: Fwd: Need reviewer: ant script bug in jaxws

2012-01-08 Thread David Holmes
Hi Kelly, On 7/01/2012 4:35 AM, Kelly O'Hair wrote: Still need a reviewer for this small change. Any volunteers? Seems okay. Did we determine how they went missing in the first place? David - -kto Begin forwarded message: From: "Kelly O'Hair" Subject: Need reviewer: ant script bug

Re: POSIX compatibility change for including wait.h in UNIXProcess_md.c

2012-01-12 Thread David Holmes
Hi Jonathon, On 12/01/2012 7:35 PM, Jonathan Lu wrote: It was found that solaris/native/java/lang/UNIXProcess_md.c includes which does not seem to be compliant with POSIX specification, in which the expected header file name should be . see http://en.wikipedia.org/wiki/C_POSIX_library. I also p

Re: Code review: 7126979 (props) JCK test java_lang/System/GetProperties.java failing [macosx]

2012-01-12 Thread David Holmes
Michael, There was a different patch for this posted earlier today where the props fields were all nulled out so they didn't reference the freed locations amy more. (I didn't keep the email) ??? David On 12/01/2012 9:22 PM, Michael McMahon wrote: Could I get the following change for jdk7u-

Re: Code review: 7126979 (props) JCK test java_lang/System/GetProperties.java failing [macosx]

2012-01-12 Thread David Holmes
atforms as similar as possible, minimising the changes with jdk7u ... - Michael. On 12/01/12 11:52, David Holmes wrote: Michael, There was a different patch for this posted earlier today where the props fields were all nulled out so they didn't reference the freed locations amy more. (I didn&#

Re: RFR 7132378: Race in FutureTask if used with explicit set ( not Runnable )

2012-01-23 Thread David Holmes
Hi Chris, Hard to evaluate a completely new design like this as the devil is always in the details. I don't understand the purpose of handlePossibleCancellationInterrupt. Given it doesn't clear the interrupt state why does it need to wait? Otherwise it looks okay. Thanks, David On 24/01/2

Re: Code review request for #6469160, #7088271

2012-01-23 Thread David Holmes
Hi Brandon, On 21/01/2012 4:19 AM, Brandon Passanisi wrote: Resending again... Hello core-libs. I was wondering of somebody could be please review the following fix for #6469160 and #7088271. The changes in the webrev fix both bugs. Information is below: Webrev URL: http://cr.openjdk.java.net/

Re: RFR 7132378: Race in FutureTask if used with explicit set ( not Runnable )

2012-01-24 Thread David Holmes
On 25/01/2012 4:56 AM, Doug Lea wrote: Sorry for delay; swamped... On 01/24/12 06:00, Chris Hegarty wrote: I don't understand the purpose of handlePossibleCancellationInterrupt. Given it doesn't clear the interrupt state why does it need to wait? Yes, this is true. I can't see any need for i

Re: Code review request for #6469160, #7088271

2012-01-25 Thread David Holmes
Hi Brandon, On 25/01/2012 5:03 PM, Brandon Passanisi wrote: Hi David. Thank you for your review comments. My answers to your questions are below: On 1/23/2012 9:56 PM, David Holmes wrote: More generally it is not clear to me that putting in this special case is the right way to fix this

Re: Code review request for #6469160, #7088271

2012-01-25 Thread David Holmes
Hi Brandon, On 25/01/2012 5:03 PM, Brandon Passanisi wrote: Hi David. Thank you for your review comments. My answers to your questions are below: On 1/23/2012 9:56 PM, David Holmes wrote: More generally it is not clear to me that putting in this special case is the right way to fix this

Re: Code review request for #6469160, #7088271: (fmt) format("%.1g", 0.0) throws ArrayOutOfBoundsException

2012-01-26 Thread David Holmes
his block of code is not run. In addition, I have re-run the Basic-X set of Formatter tests to make sure there aren't any regressions. Webrev: http://cr.openjdk.java.net/~bpassani/6469160/1/webrev/ <http://cr.openjdk.java.net/%7Ebpassani/6469160/1/webrev/> Thanks. On 1/24/2012 11:5

Re: JDK 8 code review request for 7140820 Add covariant overrides to Collections clone methods

2012-01-29 Thread David Holmes
Hi Joe, This looks okay to me. I guess we don't have any tests for that code in EventRequestSpecList! Pity about the constant need for suppressing "unchecked" :( Cheers, David On 30/01/2012 1:58 PM, Joe Darcy wrote: Hello, As an indirect outgrowth of warnings cleanup day, various categorie

Re: 7140918: Remove dependency on apt and com.sun.mirror API

2012-01-30 Thread David Holmes
On 31/01/2012 1:08 AM, Alan Bateman wrote: The following webrev is a patch from Miran Kos and Martin Grebac (cc'ed) to remove the JAX-WS dependency on apt and the com.sun.mirror API: http://cr.openjdk.java.net/~alanb/7140918/webrev/ This is needed before Joe Darcy can wield his machete. Pleas

Re: 7140918: Remove dependency on apt and com.sun.mirror API

2012-01-30 Thread David Holmes
On 31/01/2012 11:12 AM, Joe Darcy wrote: On 01/30/2012 04:50 PM, David Holmes wrote: On 31/01/2012 1:08 AM, Alan Bateman wrote: The following webrev is a patch from Miran Kos and Martin Grebac (cc'ed) to remove the JAX-WS dependency on apt and the com.sun.mirror API:

Re: java.io.File field "path" is not declared final

2012-02-15 Thread David Holmes
On 16/02/2012 11:55 AM, Weijun Wang wrote: It's almost final. That field is only changed in readObject(), which is effectively a constructor. Yes but because it is not final, if a File instance is shared and unsafely published then a thread using the File may see a wrong value for the path. P

Re: cost of Java "assert" when disabled?

2012-02-16 Thread David Holmes
The corelibs side of things seems to have gotten dropped from the cc list - added back. On 17/02/2012 8:21 AM, Vitaly Davidovich wrote: Don't want to sidetrack this thread but I really wish javac had proper conditional compilation support, which would make this issue mostly moot. But the whol

Re: cost of Java "assert" when disabled?

2012-02-16 Thread David Holmes
worry about dead IL code causing opto issues - don't see a reason why java couldn't have done the same from the beginning. Simply because the people defining the language didn't want it. I suspect there's a blog or two out there somewhere discussing this. David ----- Sent fr

Re: java.io.File field "path" is not declared final

2012-02-17 Thread David Holmes
On 17/02/2012 10:11 PM, Alan Bateman wrote: On 17/02/2012 12:00, Rémi Forax wrote: : Better with the attachment inlined :) Thanks Rémi, this looks okay to me although I probably would have used putObjectVolatile for the path field. You only need one "volatile" store and it should be the last

Re: java.io.File field "path" is not declared final

2012-02-17 Thread David Holmes
On 17/02/2012 10:41 PM, Alan Bateman wrote: On 17/02/2012 12:37, David Holmes wrote: On 17/02/2012 10:11 PM, Alan Bateman wrote: On 17/02/2012 12:00, Rémi Forax wrote: : Better with the attachment inlined :) Thanks Rémi, this looks okay to me although I probably would have used

Re: RFR : 7144488 StackOverflowError occurres on list via Collections.synchronizedList(List)

2012-02-22 Thread David Holmes
On 23/02/2012 12:22 AM, Alan Bateman wrote: On 22/02/2012 13:25, Seán Coffey wrote: Bug recently reported. We enter infinite recursion on a boundary case for Collections.synchronizedList.remove(..) Fix is a simple equals check in class method before delegating the call. bug : http://bugs.sun.c

Re: RFR : 7144488 StackOverflowError occurres on list via Collections.synchronizedList(List)

2012-02-23 Thread David Holmes
On 23/02/2012 7:55 PM, Alan Bateman wrote: On 23/02/2012 02:44, David Holmes wrote: All of the SynchronizedX.equals methods should be fixed ie SynchronizedSet, SynchronizedList and SynchronizedMap. It should always be valid to ask if a.equals(a). The idiomatic form used elsewhere (CheckedXXX

Re: RFR : 7144488 StackOverflowError occurres on list via Collections.synchronizedList(List)

2012-02-23 Thread David Holmes
On 23/02/2012 10:41 PM, Doug Lea wrote: On 02/22/12 21:44, David Holmes wrote: I'm not a fan of collections containing themselves, but I think it is simple to fix contains(o)/contains[Key]|[Value](o) and remove(o) in a similar way. Though none of the wrapped collections currently handle

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

2012-02-23 Thread David Holmes
Hi Fred, java.lang.ObjectName? :) Need to fix that. So often we intern strings precisely so that equals() can use == to improve performance. It seems to me that this is a case of "fixing" something for one use-case without knowing what the impact will be on other use-cases! Is there perhap

Re: RFR : 7144488 StackOverflowError occurres on list via Collections.synchronizedList(List)

2012-02-23 Thread David Holmes
On 24/02/2012 7:55 AM, Jason Mehrens wrote: David, For completeness, you might want to link this bug to bug id 6360946 "(coll) SetFromMap.equals should perform identity check". Most of the wrapper classes were fixed to include an identity check for that bug. Digging up some old messages from Dec

Re: RFR 7149320: Move sun.misc.VM.booted() to end of System.initializeSystemClass()

2012-02-27 Thread David Holmes
Hi Mike, The problem with changing initialization order is that you can never enumerate all the possible initialization paths. I don't see anything problematic with the move in relation to Thread.currentThread() or ThreadGroup.add(). But the call to setJavaLangAccess requires initialization o

Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method

2012-02-28 Thread David Holmes
ts contained in the returned arrays are On 07/20/2011 01:03 AM, David Holmes wrote: Just realized this has come in too late ... Joe Darcy said the following on 07/20/11 05:49: Agreed; I've posted a BlenderRev corresponding to the current patch at: http://cr.openjdk.java.net/~darcy/70075

Re: RFR 7149320: Move sun.misc.VM.booted() to end of System.initializeSystemClass()

2012-02-28 Thread David Holmes
Hi Mike, On 29/02/2012 5:23 AM, Mike Duigou wrote: On Feb 27 2012, at 20:50 , David Holmes wrote: Hi Mike, The problem with changing initialization order is that you can never enumerate all the possible initialization paths. I don't see anything problematic with the move in relati

Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method

2012-02-28 Thread David Holmes
On 29/02/2012 10:29 AM, David Holmes wrote: Hi Joe, On 29/02/2012 3:03 AM, Joe Darcy wrote: Belatedly catching up on email, please review the patch below to address the issues you've raised. I searched for "method" and replaced it with "executable" as appropriate thro

Re: RFR 7149320: Move sun.misc.VM.booted() to end of System.initializeSystemClass()

2012-02-29 Thread David Holmes
Okay - Alan has pointed out that fairly recently (6888546) setJavaLangAccess and booted() were in the opposite order to today. Based on that I now think it is extremely low risk to change the order as suggested. Thanks, David On 29/02/2012 11:15 AM, David Holmes wrote: Hi Mike, On 29/02

Re: RFR 7065380 : Allow Collections.sort to sort Collections.singletonList() result

2012-03-01 Thread David Holmes
Looks good to me. A superfluous comment in the test: 47 // 7065380 David On 2/03/2012 5:50 AM, Mike Duigou wrote: Hello all; Currently Collections.sort() refuses to sort the lists which result from calling Collections.singletonList(). This makes some sense because the singleton lis

Re: RFR 7065380 : Allow Collections.sort to sort Collections.singletonList() result

2012-03-01 Thread David Holmes
On 2/03/2012 10:22 AM, Joe Darcy wrote: Hi Mike, The main body of the javadoc of method does state 176 * The specified list must be modifiable, but need not be resizable. so I agree that a small javadoc update is needed to support this reasonable expansion of behavior. Sorry I didn't check t

Re: RFR 7065380 : Allow Collections.sort to sort Collections.singletonList() result

2012-03-01 Thread David Holmes
On 2/03/2012 10:39 AM, Kevin Bourrillion wrote: The status quo is that the user who actually experiences this problem can, at worst, replace Collections.sort(list); with if (list.size()> 1) Collections.sort(list); ... that doesn't seem so bad to me. It is horrendous if you us

Re: Suggestion about including pthread.h

2012-03-01 Thread David Holmes
On 2/03/2012 5:05 PM, Shi Jun Zhang wrote: Currently jdk/src/solaris/bin/java_md.c includes with "#ifdef __linux__", but BSD, MAC OS, AIX all needs to include pthread.h. To avoid the situation that the ifdef clause becomes longer and longer like "#if defined(__linux__) || defined(_ALLBSD_SOURCE)

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-02 Thread David Holmes
HI Charles, I tend to agree with you. In this case, in my opinion, AbstractMap.putAll has no business saying that it is equivalent to calling put() as that should be part of the implementation note, not the actual spec. Subclasses should be free to implement putAll in the most efficient manne

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-04 Thread David Holmes
he only implementation of putAll I can find from the TreeMap is using the put method. [1] The logic means: a. We have to place the implementation note in every specified method api b. The subclass feels free to change the implementation. On 03/02/2012 05:02 PM, David Holmes wrote: HI Charles, I tend t

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-05 Thread David Holmes
12:11 PM, David Holmes wrote: Hi Charles, I'm not quite sure what you are suggesting. In my opinion all that is needed is for AbstractMap.putAll to read: Copies all of the mappings from the specified map to this map (optional operation). The behavior of this operation is undefined if the sp

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-05 Thread David Holmes
FYI CR 7151065 filed. David On 5/03/2012 7:50 PM, David Holmes wrote: On 5/03/2012 7:13 PM, Charles Lee wrote: Hi David, I am sorry for the unclear. I was suggesting to add some implementation notes on the TreeMap The change as you suggested is great. The patch is @ http

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-05 Thread David Holmes
8:09 PM, David Holmes wrote: FYI CR 7151065 filed. David On 5/03/2012 7:50 PM, David Holmes wrote: On 5/03/2012 7:13 PM, Charles Lee wrote: Hi David, I am sorry for the unclear. I was suggesting to add some implementation notes on the TreeMap The change as you suggested is great. The

Re: Asking about the interesting behaviours of TreeMap.putAll

2012-03-05 Thread David Holmes
ch mapping from key k to value v in the specified map." For those two statements to have the same interpretation would require classifying the calling of a method, put(k,v), as an effect. I'd prefer if only mutations are considered effects. Mike On Mar 5 2012, at 03:10 , Doug Lea wrote: On 03

RFR XS: 7092140 Test: TimedAcquireLeak.java fails on SE-E due to -XX:-UsePerfData

2012-03-06 Thread David Holmes
webrev: http://cr.openjdk.java.net/~dholmes/7092140/webrev/ inlined: *** 146,156 final String classToCheckForLeaks = Job.classToCheckForLeaks(); final String uniqueID = String.valueOf(new Random().nextInt(Integer.MAX_VALUE)); final String[] jobC

hg: jdk8/tl/jdk: 7092140: Test: java/util/concurrent/locks/Lock/TimedAcquireLeak.java fails on SE-E due to -XX:-UsePerfData

2012-03-07 Thread david . holmes
Changeset: d4a6627d5004 Author:dholmes Date: 2012-03-08 00:46 -0500 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d4a6627d5004 7092140: Test: java/util/concurrent/locks/Lock/TimedAcquireLeak.java fails on SE-E due to -XX:-UsePerfData Summary: Add -XX:+UsePerfData to invocation of

Re: RFR XS: 7092140 Test: TimedAcquireLeak.java fails on SE-E due to -XX:-UsePerfData

2012-03-07 Thread David Holmes
Thanks Chris (and Alan), I've added a note about jsr166 cvs status. David On 7/03/2012 9:23 PM, Chris Hegarty wrote: I ok with this as is. I wonder if we should added a note about the divergence, just for future maintenance and re-syncs? -Chris. On 07/03/2012 06:55, David Holmes

Re: Suggestion about including pthread.h

2012-03-08 Thread David Holmes
On 8/03/2012 6:09 PM, Shi Jun Zhang wrote: On 3/2/2012 5:05 PM, Alan Bateman wrote: On 02/03/2012 07:53, David Holmes wrote: Yes we need to move to a more capability based inclusion & conditional compilation mechanism. I'm not sure if the build-infra project is tackling this partic

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-09 Thread David Holmes
Hi Sean, That seems to implement the required semantics. Minor style nit: }else{ -> } else { Not sure about the testcase ... Can size() not remove some elements directly but return the original size? David On 9/03/2012 6:16 PM, Sean Chou wrote: Hi all, AbstractCollection.toArray(T[]

Re: Suggestion about including pthread.h

2012-03-09 Thread David Holmes
On 9/03/2012 7:04 PM, Alan Bateman wrote: On 09/03/2012 08:01, Shi Jun Zhang wrote: The situation in NativeThread.c is more complicated than other 2 files. I'm not familiar with BSD or Mac. It seems that we don't need to signal threads on BSD or Mac. And INTERRUPT_SIGNAL on AIX will definitely b

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-09 Thread David Holmes
Here's a simple testcase that seems to work. David On 9/03/2012 7:53 PM, David Holmes wrote: Hi Sean, That seems to implement the required semantics. Minor style nit: }else{ -> } else { Not sure about the testcase ... Can size() not remove some elements directly but return the origi

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-10 Thread David Holmes
09.03.2012 12:20, schrieb David Holmes: Here's a simple testcase that seems to work. David On 9/03/2012 7:53 PM, David Holmes wrote: Hi Sean, That seems to implement the required semantics. Minor style nit: }else{ -> } else { Not sure about the testcase ... Can size() not remove some

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-10 Thread David Holmes
On 10/03/2012 12:02 PM, Ulf Zibis wrote: Am 09.03.2012 09:16, schrieb Sean Chou: Hi all, AbstractCollection.toArray(T[] ) might return a new array even if the given array has enough room for the returned elements when it is concurrently modified. This behavior violates the spec documented in ja

Re: 7152866: Tests not run because they are missing the @run tag

2012-03-11 Thread David Holmes
Hi Alan, On 11/03/2012 9:21 PM, Alan Bateman wrote: I was looking at a test and happen to notice that it had a @build tag but no @run tag so the test wasn't actually running as expected (it wasn't a compile-only test). This prompted me to do a quick audit and I found several other tests with sim

Re: 7152866: Tests not run because they are missing the @run tag

2012-03-11 Thread David Holmes
On 12/03/2012 8:12 AM, chris hegarty wrote: On 11/03/2012 20:15, David Holmes wrote: On 11/03/2012 9:21 PM, Alan Bateman wrote: I was looking at a test and happen to notice that it had a @build tag but no @run tag so the test wasn't actually running as expected (it wasn't a compile

Re: RFR 7065380 : Allow Collections.sort to sort Collections.singletonList() result

2012-03-12 Thread David Holmes
On 13/03/2012 5:08 AM, Jason Mehrens wrote: I'm not really confident about proposing "assertions as lint detection" rather than adding explicit checks. We wouldn't (and don't) use optional assertions for array bounds checking. This has clearly been the right choice. I'm still considering my>>

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-13 Thread David Holmes
Still looks okay to me. David On 13/03/2012 4:58 PM, Sean Chou wrote: Hi Ulf and David, I modified the patch and added the testcase, it's now : http://cr.openjdk.java.net/~zhouyx/7121314/webrev.02/. Ulf's compact version is used, it looks beautiful; however I replaced the Math.m

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-13 Thread David Holmes
d Éamonn On 13 March 2012 12:16, Ulf Zibis wrote: Am 10.03.2012 13:52, schrieb David Holmes: On 10/03/2012 12:02 PM, Ulf Zibis wrote: Why don't we have public T[] toArray(T[] a) ? This would prevent from the cast r[i] = (T)it.next(); It's too late to change the method sig

Re: RFR: 7155300 Include pthread.h on all POSIX platforms except Solaris to improve portability

2012-03-21 Thread David Holmes
On 21/03/2012 2:30 PM, Alan Bateman wrote: On 21/03/2012 02:47, Shi Jun Zhang wrote: Hi Alan/David, There is no response on this thread for long time. I created a sun bug 7155300, could you help to review it? The webrev link is http://cr.openjdk.java.net/~zhangshj/pthread/webrev.02/ Looks o

Re: hg: jdk8/tl/jdk: 7145454: JVM wide monitor lock in Currency.getInstance(String)

2012-03-21 Thread David Holmes
Hi, I'm sorry I missed the review of this change. The following is somewhat inefficient: instance = instances.putIfAbsent(currencyCode, new Currency(currencyCode, defaultFractionDigits, numericCode)); return (instance != null ? instance : instances.get(currencyCode));

Re: hg: jdk8/tl/jdk: 7145454: JVM wide monitor lock in Currency.getInstance(String)

2012-03-22 Thread David Holmes
Cost depends on how large and well-balanced the hashmap is. Cheers, David Naoto On 3/21/12 7:27 P, David Holmes wrote: Hi, I'm sorry I missed the review of this change. The following is somewhat inefficient: instance = instances.putIfAbsent(currencyCode, new Currency(currencyCode, defaultFr

Re: hg: jdk8/tl/jdk: 7145454: JVM wide monitor lock in Currency.getInstance(String)

2012-03-25 Thread David Holmes
On 3/22/12 6:09 P, David Holmes wrote: On 23/03/2012 1:33 AM, Naoto Sato wrote: Hi David, Sorry, the review was done in the i18n team and did not go to core libs alias. In your suggested fix, I think there is a very slight chance that two threads could get different Currency instances if the

Re: Request for review: 7156459: Remove unnecessary get() from Currency.getInstance()

2012-03-26 Thread David Holmes
Thanks Naoto! Looks good to me. David On 27/03/2012 5:01 AM, Naoto Sato wrote: Hi, Please review the following fix in java.util.Currency. This fix is to remove an unnecessary get() introduced with the prior fix to 7145454. bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7156459 webrev

Re: RFR 6963841: java/util/concurrent/Phaser/Basic.java fails intermittently

2012-03-26 Thread David Holmes
Hi Chris, On 27/03/2012 1:04 PM, Chris Hegarty wrote: David, Doug, This test has been failing intermittently on jdk7u-dev and jdk8 for a while now. It only appears to fail when run in our internal build/test system (JPRT). I believe the cause of the failure to be simply that the machines the t

Re: RFR: 7134701 [macosx] Support legacy native library names

2012-03-27 Thread David Holmes
The change to look for the alternate library name looks good to me. I'm happy to see this handled in this way. I can't comment on whether there are any other cases where the alternate name should be considered. David On 28/03/2012 12:05 AM, Michael McMahon wrote: JDK 8 fix for this issue. F

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-27 Thread David Holmes
Hi Ulf, I understand your point about ensuring we test AbstractCollection.toArray but I find this revised test much harder to understand. Also in the name setPseudoConcurrentSizeCourse the word "Course" doesn't fit. I'm not sure what you were meaning here? Perhaps just modifySize or emulate

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-28 Thread David Holmes
hment... -Ulf Am 28.03.2012 07:29, schrieb David Holmes: Hi Ulf, I understand your point about ensuring we test AbstractCollection.toArray but I find this revised test much harder to understand. Also in the name setPseudoConcurrentSizeCourse the word "Course" doesn't fit. I'

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-28 Thread David Holmes
ce flight, Thanks, it's not quite imminent but I have a lot to do in the next couple of days. :) David -Ulf Am 29.03.2012 02:36, schrieb David Holmes: Hi Ulf, Thanks for the updates. This will take a little rearranging to get into the right form I think - a single file is

Re: 7127687: MethodType leaks memory due to interning

2012-03-28 Thread David Holmes
Hi John, On 29/03/2012 9:51 AM, John Rose wrote: http://cr.openjdk.java.net/~jrose/7127687/webrev.00/ 7127687: MethodType leaks memory due to interning Summary: Replace internTable with a weak-reference version. This is a point fix for JDK 8, and will (pending approval) also be back-ported to

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-03-31 Thread David Holmes
ny * questions. */ /* * @test * @bug 7121314 * @summary AbstractCollection.toArray(T[]) doesn't return the given array * in concurrent modification. * @author Ulf Zibis, David Holmes */ import java.util.AbstractCollection; import java.util.Arrays; import java.util.Iterator; p

Re: Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

2012-04-01 Thread David Holmes
link to the start of the thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-March/009512.html On Sun, Apr 1, 2012 at 11:25 AM, David Holmes mailto:david.hol...@oracle.com>> wrote: Simplified testcase below. Let's finalise this please. Thanks, David

Re: RFR: 7134701 [macosx] Support legacy native library names

2012-04-01 Thread David Holmes
Alan, Michael, This sounds like a case where a property to control what the preferred library suffix is might be useful as a migration aid. Though the fundamental flaw in the API is assuming there is only a single suffix. David On 1/04/2012 11:16 PM, Alan Bateman wrote: On 28/03/2012 10:52,

Re: RFR 6963841: java/util/concurrent/Phaser/Basic.java fails intermittently

2012-04-02 Thread David Holmes
On 30/03/2012 9:22 PM, Chris Hegarty wrote: Sorry guys, I think I found the actual cause for this failure. Though I think the increased defensive timeouts are still a good idea. There is a race in the test itself. The "One thread interrupted" test tries to interrupt a thread blocked in awaitAdva

Initial preview: JEP-149 Reduced Class instance size

2012-04-04 Thread David Holmes
classes in an application. Please note that I've put this out just before I disappear on vacation for 10 days, so if you don't see any responses from me that is why. :) Thanks, David Holmes

Re: Initial preview: JEP-149 Reduced Class instance size

2012-04-04 Thread David Holmes
r in a ClassValue on Java SE 8, rather than a field. ClassValue is something I'm keeping an eye on, but an entry in ClassValue is going to consume more dynamic memory than a single direct field. Thanks, David On 4/4/2012 10:50 PM, David Holmes wrote: http://cr.openjdk.java.net/~dholmes/JE

Re: Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

2012-04-16 Thread David Holmes
Certainly the string management in this code is a bit of a mess, but I don't understand why the strdup's of string literals have been introduced. Even if for a good reason this seems to imply that an allocation failure will result in a NULL where before the caller was guaranteed never to get NU

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

2012-04-16 Thread David Holmes
Hi Rob, Adding new methods to Process definitely requires providing defaults. This aspect should be covered in CCC request. David On 12/04/2012 9:48 AM, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ This

Re: Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

2012-04-17 Thread David Holmes
free. It is not the free that I'm worried about. If an error occurs but the strdup fails due to a malloc failure then the caller may reference the msg. Previously this msg was never NULL but now it may be. David - On Tue, Apr 17, 2012 at 10:48 AM, David Holmes mailto:david.hol...@

Re: Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

2012-04-17 Thread David Holmes
trdup might fail. I couldn't see why you changed ZIP_Get_From_Cache but now I see that it and ZIP_Put_In_Cache have to follow the same convention regarding the error string. Sorry for the confusion. David - On Wed, Apr 18, 2012 at 10:43 AM, David Holmes mailto:david.hol...@oracle.c

Re: Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

2012-04-18 Thread David Holmes
eturn which indicates an error, and so accesses the msg and now potentially hits another NULL. It's unlikely but ... David It's webrev: http://cr.openjdk.java.net/~zhouyx/7159982/webrev.02/ <http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.02/> On Wed, Apr 18, 2012 at 1

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

2012-04-18 Thread David Holmes
On 18/04/2012 11:44 PM, Jason Mehrens wrote: Rob, It looks like waitFor is calling Object.wait(long) without owning this objects monitor. If I pass Long.MAX_VALUE to waitFor, shouldn't waitFor return if the early if the process ends? Also waitFor doesn't call wait() under the guard of a lo

Re: RFR: 7160714 - Strange or obsolete @see tags in some exception java.util javadoc

2012-04-18 Thread David Holmes
On 19/04/2012 5:49 AM, Mike Duigou wrote: Looks good with the copyright change. Agreed. Will need CCC request as well though. David - Mike On Apr 18 2012, at 12:15 , Jim Gish wrote: Description: just a trivial change to add in Iterator#next() as a @see reference (Alan and I discuss

Re: RFR: 7160725 - Strange or obsolete @see tags in some exception java.lang javadoc

2012-04-18 Thread David Holmes
Hi Jim, I guess I don't see the point here. There must be literally dozens of APIs that throw IllegalArgumentException, and a significant number that can throw NumberFormatError. What is so special about these selections? I'd argue for removing the old @see tags. David On 19/04/2012 5:47 AM

Re: Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

2012-04-18 Thread David Holmes
On 19/04/2012 4:05 AM, Alan Bateman wrote: On 18/04/2012 14:02, David Holmes wrote: On 18/04/2012 10:23 PM, Sean Chou wrote: Hi David, Alan, So is the patch acceptable ? There is still the matter of the unexpected NULL if strdup fails. I'd need to see the clients for this code to se

RFR: 7103570 AtomicIntegerFieldUpdater does not work when SecurityManager is installed

2012-04-19 Thread David Holmes
requires CCC approval which I will initiate. I have a glitch with running the test under jtreg but hopefully that will get sorted soon (jtreg fails if a security manager is installed). Thanks, David Holmes

Re: RFR: 7160725 - Strange or obsolete @see tags in some exception java.lang javadoc

2012-04-19 Thread David Holmes
Hi Jim, On 20/04/2012 12:41 AM, Jim Gish wrote: Hi David, As I discussed with Alan, I don't think the conventions for @see in general are very clear. One could argue that there are a number of approaches that could be taken, among them: an exhaustive list, a representative list, or one simpl

<    4   5   6   7   8   9   10   11   12   13   >