Re: RFR 8225198 : Optimize regex tree for greedy quantifiers of type {N, }

Hi, Ivan The change looks fine. I would call this "noreg-cleanup". Tests are needed to confirm the correctness of the new code, which I imagine we already have. One small suggestion: src/java.base/share/classes/java/util/regex/Pattern.java 4271 * and unlimited maximum, for *, + and

Re: RFR 8212807: tools/jar/multiRelease/Basic.java times out

Thank you for elaborating. The new version looks good. -Brent On 5/30/19 1:29 PM, Lance Andersen wrote: Hi Brent, On May 30, 2019, at 4:02 PM, Brent Christian mailto:brent.christ...@oracle.com>> wrote: Hi, Lance Thank you for the review. This change is to collect more infor

Re: RFR 8212807: tools/jar/multiRelease/Basic.java times out

Hi, Lance This change is to collect more information in case this happens again, yes? Looks pretty good - just a couple comments: test/jdk/tools/jar/multiRelease/Basic.java 536 jar("ufm", jarfile, manifest.toString(), Is there a reason not to convert this to call jarTool() ? --

Re: RFR 8224789 : Parsing repetition count in regex does not detect numeric overflow

Looks fine. Thanks, Ivan. -Brent On 5/29/19 7:27 AM, Roger Riggs wrote: Thanks Ivan,  Looks good. On 05/28/2019 10:08 PM, Ivan Gerasimov wrote: Hi Brent! On 5/28/19 4:06 PM, Brent Christian wrote: Hi, Ivan I agree with Roger that there are more test cases than necessary. Otherwise I

Re: RFR 8224789 : Parsing repetition count in regex does not detect numeric overflow

Hi, Ivan I agree with Roger that there are more test cases than necessary. Otherwise I think it looks pretty good. I find the addExact/multiplyExact code less readable. I'm not sure what could be done about it - maybe some different indentation: cmin = Math.addExact(

Re: RFR (CSR) - JDK-8223776 String::stripIndent (Preview)

Hi, In the description of the re-indentation algorithm, I think it's worth clarifying that the last line is always included. So perhaps: "3. The last line (i.e., the line with the text block closing delimiter) is included in the set of determining lines, even if it is blank. (The

Re: RFR 8224682: Remove the com.sun.CORBA.ORBIorTypeCheckRegistryFilter security property

+1 On 5/23/19 4:15 PM, Lance Andersen wrote: Thank you Brent for the review. On May 23, 2019, at 7:10 PM, Brent Christian mailto:brent.christ...@oracle.com>> wrote: Looks fine.  I guess you'll want to update/add the Copyright year in JDKSecurityProperties.java. Ah, forgot that,

Re: RFR 8224682: Remove the com.sun.CORBA.ORBIorTypeCheckRegistryFilter security property

Looks fine. I guess you'll want to update/add the Copyright year in JDKSecurityProperties.java. -Brent On 5/23/19 3:58 PM, Lance Andersen wrote: Hi all, Please review the fix for 8224682 which removes this legacy property left over from the removal from CORBA The webrev can be found:

Re: RFR - JDK-8223775 String::stripIndent (Preview)

Hi, Jim I have a few comments on the webrev. src/java.base/share/classes/java/lang/String.java 2982 private static int outdent(List lines) { Can you please add a doc comment for what this method does? 2973 .map(line -> { 2974 int firstNonWhitespace = line.indexOfNonWhitespace();

Re: RFR - JDK-8223775 String::stripIndent (Preview)

Hi, I have a couple questions/comments about the special treatment of the last line in a text block. -- In the CSR we have: "3. If the last line in the list of individual lines (i.e., the line with the text block closing delimiter) is blank, then add it to the set of determining lines.

Re: RFR - JDK-8223780 String::translateEscapes (Preview)

Hi, Jim I have some comments on the CSR and the Webrev. CSR: "This method takes the receiver String a replaces escape sequences with character equivalents." a -> and -- In the specification, I like emphasizing that nothing happens to 'this', but rather to the returned string, so

Re: [13] RFR: 8224105: Cannot parse JapaneseDate string on some specified locales

Thanks. Looks good to me. -Brent On 5/21/19 11:33 AM, naoto.s...@oracle.com wrote: Thanks, Brent. Modified the webrev accordingly: http://cr.openjdk.java.net/~naoto/8224105/webrev.01/ Naoto On 5/21/19 11:03 AM, Brent Christian wrote: Hi, Naoto.  I have a couple comments. src/java.base

Re: [13] RFR: 8224105: Cannot parse JapaneseDate string on some specified locales

Hi, Naoto. I have a couple comments. src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java String.isEmpty() could be used in place of equals(""). test/jdk/java/time/test/java/time/chrono/TestEraDisplayName.java Maybe give the new constants names in all-caps.

Re: RFR 8223730 : URLClassLoader.findClass() can throw IndexOutOfBoundsException

I think the change looks OK. I agree that this case is unlikely to come up in the real world, so no regtest seems acceptable; tag the bug with noreg-hard. (Another option might be a test in a seldom-run Tier that @requires a large amount of heap.) -Brent On 5/11/19 3:07 PM, Ivan Gerasimov

Re: RFR (M): JDK-6394757: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

Hi, Stuart. That all looks pretty good to me. I think, "membership semantics" is a good term. Just a few minor comments: Collection.java: 110 * that use different membership semantics. For operations that involve more than 111 * one collection, it is specified which collection's

RFR 8221267 : Document the jdk.net.URLClassPath.showIgnoredClassPathEntries system property

Hi, The "jdk.net.URLClassPath.showIgnoredClassPathEntries" system property was added by 8211941[1]. When enabled, it provides debugging output of invalid "Class-Path" attributes in JAR manifests (which are ordinarily silently ignored). The new property will be mentioned in the JDK 13

Re: [OpenJDK 2D-Dev] RFR: 8130266: Change the mechanism by which JDK loads the platform-specific GraphicsEnvironment class

The java.base portion looks good to me. -Brent On 4/25/19 1:32 PM, Philip Race wrote: And the complete CSR link is this : https://bugs.openjdk.java.net/browse/JDK-8222990 On 4/25/19, 1:31 PM, Philip Race wrote: Sorry, meant to include core-libs on this ! -phil. On 4/25/19, 1:12 PM, Phil

Re: RFR 8211941 : Enable checking/ignoring of non-conforming Class-Path entries

to document this JDK-specific property? thanks Mandy On 3/20/19 2:01 PM, Brent Christian wrote: Hi, JDK-8216401[1][2] added code to improve JAR spec conformance (WRT relative URLs in the Class-Path attribute), while maintaining some key compatibility aspects (entries using "file:&

RFR 8211941 : Enable checking/ignoring of non-conforming Class-Path entries

Hi, JDK-8216401[1][2] added code to improve JAR spec conformance (WRT relative URLs in the Class-Path attribute), while maintaining some key compatibility aspects (entries using "file:" URLs are not relative, but are used by some libraries). This code was disabled by default, but we would

Re: RFR 8220088 : Test "java/lang/annotation/loaderLeak/Main.java" fails with -Xcomp

On 3/14/19 10:41 AM, Brent Christian wrote: On 3/13/19 6:08 PM, Martin Buchholz wrote: >> Why not Reference.reachabilityFence ? http://cr.openjdk.java.net/~bchristi/8220088/webrev.01/ This looks good now, yes? -Brent

Re: RFR 8220088 : Test "java/lang/annotation/loaderLeak/Main.java" fails with -Xcomp

On 3/13/19 6:08 PM, Martin Buchholz wrote: Why not Reference.reachabilityFence ? You mean the mechanism for this precise situation. Yeah, OK. http://cr.openjdk.java.net/~bchristi/8220088/webrev.01/ Thanks, -Brent

RFR 8220088 : Test "java/lang/annotation/loaderLeak/Main.java" fails with -Xcomp

Hi, Please review my test-only fix for 8220088[1]. Along with fixing the issue, I've also converted this .sh test to .java. Webrev is here: http://cr.openjdk.java.net/~bchristi/8220088/webrev.00/ The test does the following: 1. sets up a classloader and loads class "C" into a WeakReference

Re: RFR 8220005: java/util/Arrays/TimSortStackSize2.java times out

+1 -Brent On 3/12/19 10:30 AM, Lance Andersen wrote: Hi all, Please review this addition to the ProblemList.txt as this tests times out from time to time $ hg diff test/jdk/ProblemList.txt diff -r 687e10fefa11 test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt Mon Mar 11 13:37:56

Re: [PATCH] 8218268: Javac treats Manifest Class-Path entries as Paths instead of URLs

Hi, Jon On 2/28/19 1:38 PM, Jonathan Gibbons wrote: On 28/02/2019 20:58, Jonathan Gibbons wrote: Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader: |The resulting URLs are

Re: Missing Blank Line Suspected in jar.html

Hi, Philipp Yes, I agree it would make sense to add a newline in the example Manifest, as you suggest. I can look into changing it. Thanks, -Brent On 2/12/19 9:05 AM, Philipp Kunz wrote: Hi, https://docs.oracle.com/javase/10/docs/specs/jar/jar.html#per-entry-att ributes shows an example

Re: RFR 8080569: (process) java/lang/ProcessBuilder/DestroyTest.java fails with "Process terminated prematurely"

Neat! +1 -Brent On 1/14/19 12:05 PM, Roger Riggs wrote: Hi, Thanks for the reviews. As suggested, cleaned up a bit more dead wood. http://cr.openjdk.java.net/~rriggs/webrev-destroytest-8080569-4/ Thanks, Roger On 01/14/2019 01:56 PM, Brent Christian wrote: Hi, Roger On Windows

Re: RFR 8080569: (process) java/lang/ProcessBuilder/DestroyTest.java fails with "Process terminated prematurely"

Hi, Roger On Windows, the test did not check liveness, but will check it now; seems desirable. I think the changes look fine as they are. Additional refactoring possibilities for your consideration, to take or leave: * ProcessTest::isAlive() is not used * killProc() no longer needs

RFR 8216401 : Allow "file:" URLs in Class-Path of local JARs

Hi, Please review my change for 8216401[1]. Webrev: http://cr.openjdk.java.net/~bchristi/8216401/webrev-01/ The Class-Path section of the JAR spec[2] states that entries must be relative URLs, however this hasn't been enforced for most of Java's history. There is now code to enforce this,

Re: RFR 8216205: Java API documentation formatting error in System.getEnv

passing information to a Java subprocess, On 01/07/2019 02:03 PM, Brent Christian wrote: Also, AFAICT all the other javadoc mentions of RuntimePermission (besides "getenv") use @code, rather than @link. -Brent On 1/7/19 10:58 AM, Brent Christian wrote: Looks good, thoug

Re: RFR 8216205: Java API documentation formatting error in System.getEnv

Also, AFAICT all the other javadoc mentions of RuntimePermission (besides "getenv") use @code, rather than @link. -Brent On 1/7/19 10:58 AM, Brent Christian wrote: Looks good, though it looks like a similar change is needed on L987 for System.getenv(name). -Brent On 1/7/19 10:45

Re: RFR 8216205: Java API documentation formatting error in System.getEnv

Looks good, though it looks like a similar change is needed on L987 for System.getenv(name). -Brent On 1/7/19 10:45 AM, Roger Riggs wrote: Please review a javadoc fix. Only @link is needed, @code is removed. diff --git a/src/java.base/share/classes/java/lang/System.java

Re: 8216134 (process) ProcessBuilder startPipeline does not hide piped streams

Looks good. If you wanted to break up L278 in the test before pushing, I wouldn't complain. :) -Brent On 1/4/19 6:52 AM, Roger Riggs wrote: Hi Brent, Steve, Thanks for the review and corrections for copyrights, removing debugging info, and input file contents. Updated Webrev:  

Re: 8216134 (process) ProcessBuilder startPipeline does not hide piped streams

2:37 PM, Brent Christian wrote: Speaking of, you might want to change the copyright years to 2019. :)

Re: 8216134 (process) ProcessBuilder startPipeline does not hide piped streams

Speaking of, you might want to change the copyright years to 2019. :) -Brent On 1/3/19 2:32 PM, Roger Riggs wrote: Happy New Year On 01/03/2019 05:27 PM, Lance Andersen wrote: Happy New Year On Jan 3, 2019, at 3:47 PM, Roger Riggs Happy New Year!

Re: RFR: 8215000: tools/launcher/JliLaunchTest.java fails on Windows

Hi, Shouldn't the lambdas be checking for v == null, rather than k == null? -Brent On 12/12/18 9:36 AM, Henry Jen wrote: Hi, Can I get a review of following patch? Looks like the assumption test jdk will be in PATH is simply not true, jtreg doesn’t do that. Also, this patch make sure the

Re: RFR 4947890 : Minimize JNI upcalls in system property initialization

Hi, On 11/19/18 3:37 PM, Roger Riggs wrote: Raw::xxx_NDX are initialized to 1 + previous_NDX.  It's a general good approach to increment the index but I find it error-prone and hard to catch mistake since the (adjacent) variable names look so alike. Perhaps some form of verification or

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

Do we also want an @see in skip(), pointing to skipNBytes() ? Thanks, -Brent On 11/16/18 11:14 AM, Brian Burkhalter wrote: --- a/src/java.base/share/classes/java/io/InputStream.java +++ b/src/java.base/share/classes/java/io/InputStream.java @@ -579,6 +579,7 @@ * when this

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

Hi, Roger * I like Mandy's idea of not combining the cli/VM props and the well-known props into a single array. Maybe we could then avoid copying between arrays (System.c L166). * The name "Raw" doesn't really speak to me. It's OK as an inner class, though I wonder if everything could be

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

Along the same lines, if subclasses would typically benefit from overriding both skip() and skipNBytes(), how about we call out skipNBytes() in the skip() docs? Either add a @see, or maybe append: "Subclasses are encouraged to provide a more efficient implementation of this method" + " (as

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

On 11/9/18 2:04 PM, Brian Burkhalter wrote: An updated patch is at http://cr.openjdk.java.net/~bpb/6516099/webrev.06/ including a revision of the implementation to align with the words. The tests are not updated yet. I think this looks

Re: RFR 8185496: Improve performance of system properties initialization in initPhase1

Hi, Roger The changes look good to me (FWIW - I know it was already pushed :). -Brent On 11/8/18 7:41 AM, Roger Riggs wrote: Webrev updated in place:  (Only System.java is modified) http://cr.openjdk.java.net/%7Erriggs/webrev-props-cleanup-8185496/index.html

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

On 11/8/18 12:52 PM, Brian Burkhalter wrote: Hi Daniel, On Nov 8, 2018, at 1:50 AM, Daniel Fuchs wrote: So FWIW my point was that there's nothing that you can really guarantee in InputSteam::skipFully() if a subclass implementation of skip() uses negative number to e.g. signal abnormal

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

Oct 23, 2018, at 11:29 AM, Brian Burkhalter mailto:brian.burkhal...@oracle.com>> wrote: That is exactly why version .00 of the patch implemented skipNBytes() in a fixed way in terms of read(). I think that there is a good compromise however and I shall update the patch accordingly. On Oct

Re: [12] RFR of JDK-8210908: Refactor java/util/prefs/PrefsSpi.sh to plain java test

Super - thanks, Amy. -Brent On 10/23/18 7:07 PM, Amy Lu wrote: Thank you Brent for the comments! All fixed in the new webrev: http://cr.openjdk.java.net/~amlu/8210908/webrev.01/ Thanks, Amy On 2018/10/24 4:39 AM, Brent Christian wrote: Hi, Amy I think this looks quite good

Re: [12] RFR of JDK-8209768: Refactor java/util/prefs/CheckUserPrefsStorage.sh to plain java test

Looks great. -Brent On 10/24/18 2:04 AM, Amy Lu wrote: Please review the updated version: http://cr.openjdk.java.net/~amlu/8209768/webrev.01/ Thanks, Amy Other opinions? Thanks, -Brent On 10/22/18 8:46 PM, Amy Lu wrote: Please review. Thanks, Amy On 2018/10/11 4:47 PM, Amy Lu

Re: [12] RFR of JDK-8210908: Refactor java/util/prefs/PrefsSpi.sh to plain java test

Hi, Amy I think this looks quite good as it is. Just a couple very minor comments, that you can take or leave: * I don't think a List is needed for the 'expected' argument, it can just be a String. The @DataProvider should be able to return Object[]s of a List and a String. * Really,

Re: [12] RFR of JDK-8209768: Refactor java/util/prefs/CheckUserPrefsStorage.sh to plain java test

Hi, Amy So one difference from the original test is that the process that creates the preferences store is still active at the time that the preference is checked. I don't know if this is material to what is being tested or not. It would be more similar to the original test to keep

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

Hi, Brian 562 public void skipNBytes(long n) throws IOException { 563 if (n > 0 && skip(n) != n) { 564 throw new EOFException("End of stream before enough bytes skipped"); 565 } 566 } If an overrided skip() method were to skip < n bytes but not yet be

Re: RFR 12 : 8072130 : java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Mac OSX

, Brent Christian wrote: Any thoughts on this change?  -B On 9/11/18 3:41 PM, Brent Christian wrote: Hi, Please review this change to how the platform encoding is determined on Mac when loading agents. Webrev: http://cr.openjdk.java.net/~bchristi/8072130/webrev.01.cleanned/ Additional details

Re: RFR 12 : 8072130 : java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Mac OSX

Any thoughts on this change? -B On 9/11/18 3:41 PM, Brent Christian wrote: Hi, Please review this change to how the platform encoding is determined on Mac when loading agents. Webrev: http://cr.openjdk.java.net/~bchristi/8072130/webrev.01.cleanned/ Additional details in the bug report

Re: RFR(XS) : 8210779 : 8182404 and 8210732 haven't updated copyright years

Looks like you got them all - reviewed. -Brent On 09/14/2018 04:09 PM, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8210779/webrev.00/index.html 36 lines changed: 0 ins; 0 del; 36 mod; Hi all, could you please review this small and trivial follow-up fix for 8182404 and

RFR 12 : 8072130 : java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Mac OSX

Hi, Please review this change to how the platform encoding is determined on Mac when loading agents. Webrev: http://cr.openjdk.java.net/~bchristi/8072130/webrev.01.cleanned/ Additional details in the bug report: https://bugs.openjdk.java.net/browse/JDK-8072130 Some background: Since JDK 8,

Re: [12] (AIX) 8207744: Clean up inconsistent use of opendir/closedir versus opendir64/closedir64

Hi, Brian The change looks fine to me. From my search, I believe all usages have been covered. -Brent On 8/24/18 11:01 AM, Brian Burkhalter wrote: This one could still use a Reviewer approval or rejection. Thanks, Brian On Aug 14, 2018, at 1:34 PM, Brian Burkhalter wrote: Hi Bernard,

Re: RFR(XXS) : 8209740 : typo in test/lib/jtreg/SkippedException.java

Looks fine. -Brent On 8/20/18 11:29 AM, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8209740/webrev.00/index.html 1 line changed: 0 ins; 0 del; 1 mod; Hi all, could you please review this trivial fix for the typo in SkippedException.java ? Thank Martin B for reporting the

Re: [12]RFR 8205399 : Set node color on pinned HashMap.TreeNode deletion

://download.java.net/java/early_access/jdk12/docs/api/java.base/java/lang/Integer.html#compare(int,int) Approved. On Wed, Aug 8, 2018 at 11:57 AM, Brent Christian mailto:brent.christ...@oracle.com>> wrote: Hi, Please review the following fix. Bug: https://bugs.openjdk.java.net/brow

[12]RFR 8205399 : Set node color on pinned HashMap.TreeNode deletion

Hi, Please review the following fix. Bug: https://bugs.openjdk.java.net/browse/JDK-8205399 Webrev: http://cr.openjdk.java.net/~bchristi/8205399/webrev01/ The vmTestbase/vm/gc/containers/Combination05/TestDescription.java test, which does random adds and removes (via Iterator), has been

Re: RFR: 8207005 : Disable the file canonicalization cache by default

On 07/11/2018 08:52 AM, Alan Bateman wrote: > Happy to see this disabled as it has always been problematic. The patch looks okay although you don't of couse need to set the initial values to false. OK, yeah, I'll omit that. Thanks, Alan and Brian. -Brent

RFR: 8207005 : Disable the file canonicalization cache by default

Hi, Please review this change to disable the file canonicalization cache by default. Bug: https://bugs.openjdk.java.net/browse/JDK-8207005 From the bug report: "The file canonicalization cache was added back in JDK 1.4.2 in order to improve startup time. The cache has long-standing

Re: RFR JDK-8066709 Make some JDK system properties read only

On 6/19/18 8:08 AM, Roger Riggs wrote: * src/java.base/share/classes/java/lang/System.java : Should the @implNote with the list of cached properties be added everywhere the @apiNote is being added ?  Right now the @implNote is only added to getProperties(). The repetition was getting

Re: RFR JDK-8066709 Make some JDK system properties read only

Hi, Roger On 6/18/18 7:31 AM, Roger Riggs wrote: The CSR and Webrev are updated. webrev:   http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709 * src/java.base/share/classes/java/lang/System.java : Should the @implNote with the list of cached properties be added everywhere

Re: RFR 8199435 : Unsafe publication of java.util.Properties.map

On 6/15/18 10:35 AM, Paul Sandoz wrote: I would also publish the map in readHashtable after you have placed in the keys/values. Like this? // create CHM of appropriate capacity -map = new ConcurrentHashMap<>(elements); +ConcurrentHashMap tmpMap = new

RFR 8199435 : Unsafe publication of java.util.Properties.map

Hi, In JDK 9, 8029891[1] refactored java.util.Properties to store its values in an internal ConcurrentHashMap, and removed synchronization from "reader" methods in order to avoid potential hangs/deadlocks during classloading. Claes has noticed that there is the possibility of the new 'map'

Re: RFR 8204565 : (spec) Document java.{vm.}?specification.version system properties' relation to $FEATURE

On 6/8/18 12:27 PM, mandy chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8204565/webrev/ test/jdk/java/lang/System/Versions.java   it can also verify java.vm.specification.version. The hotspot test looks to me that it should expect the test be run with OpenJDK build and the

Re: RFR 8204565 : (spec) Document java.{vm.}?specification.version system properties' relation to $FEATURE

On 6/7/18 1:24 PM, mandy chung wrote: Issue: https://bugs.openjdk.java.net/browse/JDK-8204565 Webrev: http://cr.openjdk.java.net/~bchristi/8204565/webrev/ Is there an existing test validating this? Looks like there is (kind of), for libs and for hotspot. I've rev'ed the webrev in place

RFR 8204565 : (spec) Document java.{vm.}?specification.version system properties' relation to $FEATURE

Hi, Please review this doc-only change. From the bug report: 'With the integration of JEP 322 "Time-Based Release Versioning" into JDK 10, VERSION_FEATURE is used to set the value of the system properties "java.specification.version" [1] and "java.vm.specification.version" [2] (though the

Re: RFR JDK-8066709 Make some JDK system properties read only

Hi, Roger A few things I noticed in src/java.base/share/classes/java/lang/System.java: * Properties can also be set via setProperties(Properties p), though that method is not mentioned in the doc changes (other than to setProperties() itself). 750 * setting a standard property after

Re: RFR(XS) [closed] : 8198924: java/lang/StackWalker/LocalsAndOperands.java timeouts with Graal

Looks good, Katya - thanks. I agree with omitting @bug and @summary from the second @test tag. Thanks, -Brent On 3/6/18 1:59 PM, mandy chung wrote: Running #1 and #2 when Graal is enabled is fine. For the second @test, does it need @bug and @summary to run?  If not, I suggest to take it out

Re: [11] RFR JDK-8168682: jdk/test/java/lang/ClassLoader/forNameLeak/ClassForNameLeak.java fails with -Xcomp

Hi, Mandy Interesting problem. Your changes look fine. (The static finals could be all caps, but I wouldn't bother with further changes just for that.) Thanks, -Brent On 1/24/18 10:15 AM, mandy chung wrote: This patch revises the test to make the phantom reference reachable in order to

Re: [11] RFR 8194879 : Runtime.Version parses string which does not conform to spec without throwing IAE

Done[1] - thanks Roger. I also updated the copyright years. -Brent 1. http://cr.openjdk.java.net/~bchristi/8194879/webrev.01/ On 1/17/18 7:04 AM, Roger Riggs wrote: Hi Brent, The bug number should be added to the @bug line in the test. Thanks, Roger On 1/16/2018 4:21 PM, Brent Christian

[11] RFR 8194879 : Runtime.Version parses string which does not conform to spec without throwing IAE

Hi, Please review this version string parsing fix for Runtime.Version. Issue: https://bugs.openjdk.java.net/browse/JDK-8194879 Webrev: http://cr.openjdk.java.net/~bchristi/8194879/webrev.00/index.html From the bug: -- "As per the specification of Runtime.Version: A version string, $VSTR, is a

Re: RFR[10] 8193460 : Take tools/launcher/TestXcheckJNIWarnings.java back off the ProblemList

On 12/13/17 8:59 PM, David Holmes wrote: I'm going to manually import the missing changes from jdk/hs to jdk/jdk. Unfortunately this was not possible as the changeset for 8190984 conflicts with this changeset (8193460). So the test will fail again in jdk/jdk until the changes make their

Re: RFR[10] 8193460 : Take tools/launcher/TestXcheckJNIWarnings.java back off the ProblemList

On 12/13/17 3:17 PM, David Holmes wrote: I already took care of this as part of the fix for 8190984. OK, thanks. Well, crud - my apologies to the person doing the merge later. :\ (I saw that the push went to jdk/hs, but didn't look closely. I guess you must have updated from jdk/jdk

Re: RFR[10] 8193460 : Take tools/launcher/TestXcheckJNIWarnings.java back off the ProblemList

"Nimble" is one word for it... :D -B On 12/13/17 10:54 AM, joe darcy wrote: Good to see nimble usage of the problem list;,helps keep our test results clean all the time :-) -Joe

Re: RFR[10] 8193460 : Take tools/launcher/TestXcheckJNIWarnings.java back off the ProblemList

Thank you, Paul and Mandy. -B

RFR[10] 8193460 : Take tools/launcher/TestXcheckJNIWarnings.java back off the ProblemList

Hi, Way back five days ago[1], TestXcheckJNIWarnings.java was added to the ProblemList. Well, with a couple of recent fixes[2][3], this test can be taken back off of the ProblemList. --- a/test/jdk/ProblemList.txt Fri Dec 08 13:04:43 2017 -0800 +++ b/test/jdk/ProblemList.txt Wed Dec 13

Re: RFR[10] 8190984 : tools/launcher/TestXcheckJNIWarnings.java WARNING was found in the output

nks, David Mandy On 12/12/17 4:00 PM, David Holmes wrote: Reviewed! :) Thanks Brent! I guess I can push these both now. David On 13/12/2017 9:38 AM, Brent Christian wrote: Hi, Please review this small change to the System.initProperties() native method. The tool

RFR[10] 8190984 : tools/launcher/TestXcheckJNIWarnings.java WARNING was found in the output

Hi, Please review this small change to the System.initProperties() native method. The tools/launcher/TestXcheckJNIWarnings.java has begun to fail due to this warning being issued: WARNING: JNI local refs: 41, exceeds capacity: 40 at java.lang.System.initProperties(java.base/Native

Re: RFR of 8180451: ByteArrayInputStream should override readAllBytes, readNBytes, and transferTo

Hi, Brian The changes look fine to me. I would have found the test case a little easier to follow if "off"/"len" weren't named so similarly to "offset"/"length", but it's not a big deal. -Brent On 12/11/17 12:47 PM, Brian Burkhalter wrote: https://bugs.openjdk.java.net/browse/JDK-8180451

Re: RFR 4358774: Add null InputStream and OutputStream

Hi, Brian On 12/8/17 3:12 PM, Brian Burkhalter wrote: http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ The changes look good to me. I just noticed a couple small things in the test: test/jdk/java/io/InputStream/NullInputStream.java 109 @Test(groups = "open") 110 public

Re: RFR 8193271 : ProblemList tools/launcher/TestXcheckJNIWarnings.java

Thanks, Joe -B On 12/8/17 11:45 AM, joe darcy wrote: +1 Cheers, -Joe On 12/8/2017 11:44 AM, Brent Christian wrote: Hi, Please review my change to add the intermittently-failing tools/launcher/TestXcheckJNIWarnings.java to the problem list. diff -r a1f88c937a77 test/jdk

RFR 8193271 : ProblemList tools/launcher/TestXcheckJNIWarnings.java

Hi, Please review my change to add the intermittently-failing tools/launcher/TestXcheckJNIWarnings.java to the problem list. diff -r a1f88c937a77 test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt Tue Nov 28 10:15:47 2017 -0800 +++ b/test/jdk/ProblemList.txt Fri Dec 08 11:42:55 2017

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

On 12/1/17 5:22 PM, Mandy Chung wrote: > > I think should also rethrow the cause if the cause is an Error > (why not) Alright. The instanceof RuntimeException check should be moved outside to the if-statement when it’s an instance of InvocationTargetException. So rethrow RuntimeExceptions

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

On 12/1/17 11:40 AM, Brent Christian wrote: On 12/1/17 8:33 AM, mandy chung wrote: Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with. Yes

Re: RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

On 12/1/17 8:33 AM, mandy chung wrote: Better still might be for initSystemClassLoader to re-throw the cause so that it appears immediately after the "Error occurred during initialization of VM" message that the VM will fail with. Yes that would be better. So would I do this for

RFR 8187222 : ClassLoader.getSystemClassLoader not clear if recursive initialization leads to ISE or unspecified error

Hi, Please review the following change: Bug: https://bugs.openjdk.java.net/browse/JDK-8187222 Webrev: http://cr.openjdk.java.net/~bchristi/8187222/webrev.00/ The method description of ClassLoader.getSystemClassLoader() states (in regards to setting a custom system classloader): "If circular

RFR 8191173 : (cl) Clarify or remove "for delegation" in ClassLoader spec

Hi, Please review my change to this small bit of ClassLoader spec that can be tidied up, as noticed by Martin. Issue: https://bugs.openjdk.java.net/browse/JDK-8191173 Webrev: http://cr.openjdk.java.net/~bchristi/8191173/webrev.00/ In java.lang.ClassLoader these methods: getParent()

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

On 11/07/2017 09:45 AM, mandy chung wrote: StackFrameInfo.java 38 // Footprint improvement: MemberName::clazz can replace 39 // StackFrameInfo::declaringClass. The above comment can be removed. Whoops - thanks. 41 private final boolean retainClassRef; JVMS [1] has a note

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

, the VM will put both bci and retainClassRef on the same 32 bits slot. cheers, Rémi - Mail original - De: "Brent Christian" <brent.christ...@oracle.com> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "hotspot-dev" <hotspot-...@openjdk.

RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

Hi, There are a couple opportunities to reduce the memory footprint of java.lang.StackFrameInfo (the internal implementation of java.lang.StackWalker.StackFrame): 8153682[1] : StackFrameInfo.declaringClass could be removed 8185925[2] : StackFrameInfo::walker field can be replaced with bitmap

Re: RFR 8189319 : Add a java.util.Properties constructor that takes an initial capacity

Thank you for the reviews! -Brent

Re: RFR 8189319 : Add a java.util.Properties constructor that takes an initial capacity

On 10/27/2017 11:19 AM, mandy chung wrote: It may be cleaner to initialize the map in a single place e.g. a private constructor taking Properties and initialCapacity. Yeah, that's a good idea. See new webrev: http://cr.openjdk.java.net/~bchristi/8189319/webrev.02/index.html Thanks, -Brent

RFR 8189319 : Add a java.util.Properties constructor that takes an initial capacity

Hi, It would be useful to have a Properties constructor that takes an argument to set the initial capacity. Such a constructor is present on many of the other Map implementations in the JDK, including Hashtable, the superclass of Properties. In particular, being able to specify the initial

RFR: 8183901 : Fix broken links to "Package Sealing" in the JAR spec

Hi, Please review my fix of a few broken links to the "Package Sealing" section of the Jar Spec[1]. http://cr.openjdk.java.net/~bchristi/8183901/ Thanks, -Brent 1. https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#package-sealing

Re: RFR 8187772 : JVM crash when currency set on MacOS 10.10 and earlier

Thanks, Naoto. An automated test run looked fine, so I've pushed this change. -Brent On 10/12/17 12:38 PM, Naoto Sato wrote: Looks fine, Brent. Naoto On 10/12/17 11:52 AM, Brent Christian wrote: Hi, Please review my change to prevent a startup crash on earlier versions of MacOS

Re: RFR 8187772 : JVM crash when currency set on MacOS 10.10 and earlier

n 10/12/17, 11:52 AM, Brent Christian wrote: Hi, Please review my change to prevent a startup crash on earlier versions of MacOS.    Bug: https://bugs.openjdk.java.net/browse/JDK-8187772 Webrev: http://cr.openjdk.java.net/~bchristi/8187772/webrev.00/ When a non-default currency is set in t

RFR 8187772 : JVM crash when currency set on MacOS 10.10 and earlier

Hi, Please review my change to prevent a startup crash on earlier versions of MacOS. Bug: https://bugs.openjdk.java.net/browse/JDK-8187772 Webrev: http://cr.openjdk.java.net/~bchristi/8187772/webrev.00/ When a non-default currency is set in the Language & Region control panel, it's

Re: Review Request JDK-8164512: Replace ClassLoader use of finalizer with phantom reference to unload native library

Hi, Mandy The changes look alright to me. One thing that I noticed: ClassLoader.NativeLibrary.register() writes to 'loadedLibraryNames', 'nativeLibraries', and 'systemNativeLibraries' without first synchronizing on them. There is not a bug here per se, as register() is called from inside

Re: Review Request JDK-8186050: StackFrame should provide the method signature

Looks good. I have only one (minor) comment. In the new Javadoc for getMethodType(): StackWalker.java: 133 * Returns {@link MethodType} representing the parameter types and The docs for similar methods all begin with, "Returns _the_ ..." -Brent On 08/31/2017 10:39 PM, mandy

Re: Review Request JDK-8186050: StackFrame should provide the method signature

For providing something informational, option #3 is a good choice. It's simple (no need for new StackWalker access options), yet sufficient. The changes look good to me. Thanks, -Brent On 08/28/2017 07:37 PM, mandy chung wrote: On 8/28/17 4:23 PM, Paul Sandoz wrote: Hi Mandy,

Re: RFR [10] 8186217 : Remove erroneous @hidden JavaDoc tag from java.util.Properties.replace(Object, Object, Object)

Thanks! -B On 08/25/2017 10:29 AM, Brian Burkhalter wrote: +2 On Aug 25, 2017, at 10:28 AM, Naoto Sato <naoto.s...@oracle.com <mailto:naoto.s...@oracle.com>> wrote: +1 Naoto On 8/25/17 10:03 AM, Brent Christian wrote: Hi, Please review this simple doc fi

<    1   2   3   4   5   >