Re: RFR(s): 8034999 change rmidRunning to a simple lookup (RMI test library)

[Joseph Darcy]

Hi Stuart,

Looks good to go back; thanks,

-Joe

On 2/26/2014 4:44 PM, Stuart Marks wrote:

Hi all,

Any takers for this review?

s'marks

On 2/18/14 6:47 PM, Stuart Marks wrote:

Hi all,

Please review this change to remove a redundant timing-retry loop 
from the
rmidRunning() routine of the RMI test library. It is replaced with a 
simple rmid

registry lookup that returns status immediately, without retries.

Bug:

 https://bugs.openjdk.java.net/browse/JDK-8034999

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8034999/webrev.0/

Thanks,

s'marks

Re: Serial warnings in java.util.ArrayPrefixHelpers

[Joseph Darcy]

On 2/19/2014 12:16 PM, Martin Buchholz wrote:

The jsr166 CVS version of this file already has serialVersionUIDs added.
jsr166 CVS src/main has been jdk8/jdk9 warning-clean for a while now.


Thanks for the update Martin; looking forward to the next sync to get 
rid of a few more warnings :-)


-Joe



http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/ArrayPrefixHelpers.java?view=markup

yeah, we need to do a better job of down-streaming...


On Wed, Feb 19, 2014 at 10:25 AM, Joe Darcy > wrote:


Hello,

The file java.util.ArrayPrefixHelpers seems to be part of the
166-alpha set of files in the JDK.

There are a number of package-private static final and
serializable classes in this file which neither define
serialVersionUIDs nor have a @SuppressWarnings("serial")
annotation. So, this file adds a number of serial warnings to the
JDK and efforts are underway to clear the jdk repo of serial
warnings, which are mostly in the client area:

JDK-8032976: Fix serial lint warnings in client libraries

Can the maintainers of ArrayPrefixHelpers "do the right thing"
with respect to the serial warnings in this file? From a quick
look at the file, it wasn't clear to me if SUIDs should be added
or warnings suppressed.

Thanks,

-Joe

Re: RFR(XS): 8032678: [TESTBUG] sun/misc/Version/Version.java doesn't understand two-digit HotSpot minor version numbers

[Joseph Darcy]

I've removed the incorrect-for-JBS multiple release values of "8-pool, 
9" in the fixVersion field of the main bug record.


-Joe

On 1/24/2014 10:39 AM, Volker Simonis wrote:

Hi Vladimir,

I've just pushed this to jdk9/dev.

I also received an email stating that "this bug needs to be updated,
please request to defer or upgrade to P1 and request for approval". I
assume that's because I entered "8-pool, 9" as "Fix versions".

Now I actually don't know what to do. It seems strange to upgrade this
to P1 because it is only a test bug. On the other hand it is needed in
8u because there's where we have the trouble.

So should I upgrade it to P1 and request approval for jdk8u-dev (or
directly jdk8u)? The actual problem occurs in jdk8u only, because
jdk8u-dev doesn't yet has change 8032678 which bumps the HS minor
number to 20.

Can I push directly to jdk8u or does the change has to go trough jdk8u-dev?

Thanks,
Volker

On Fri, Jan 24, 2014 at 6:13 PM, Vladimir Kozlov
 wrote:

Good change. I got this failure too.

Thanks,
Vladimir


On 1/24/14 9:04 AM, Volker Simonis wrote:

Hi,

could you please review the following trivial change:

http://cr.openjdk.java.net/~simonis/webrevs/8032678/

which fixes the JDK test jdk/test/sun/misc/Version/Version.java to
correctly handle two-digit HotSpot minor version numbers. This became
necessary after change "8031552: Update the Hotspot version numbers in
Hotspot for JDK 8U" bumped the minor HS version to '20' in jdk8u.

This webrev is for jdk9 but it will have to be backported to jdk8u
because there’s where the current problem happens.

Thanks,
Volker

Re: JDK 9 RFC on 6667086: Double.doubleToLongBits(final double value) contains inefficient test for NaN

[Joseph Darcy]

Hi Brian,

On 1/16/2014 10:51 AM, Brian Burkhalter wrote:

Hi Joe,

On Jan 16, 2014, at 9:23 AM, Joe Darcy wrote:


A few comments here. If you are making this change in Double, you would make 
the corresponding change in Float too.

Please see the updated webrev http://cr.openjdk.java.net/~bpb/6667086/webrev.2/.


New webrev looks good.




Some explanation on why I wrote these methods with the integer-field-based null 
check, some processors are implemented such that operating on the IEEE 
non-finite value of NaN and +/- infinity runs much more slowly than operating 
on normal value, sometimes orders of magnitude more slowly.

For the bitwise conversion methods, since we were biting the bullet to do the 
conversion anyway, I thought I would avoid the worst-case cost of tripping over 
a NaN by doing the NaN check on the integer value.

Thanks for the explanation. Would {Double,Float}.isNaN() perhaps be good 
candidates for VM intrinsics?


I don't think so. The canonical isNaN idiom is just (x != x) so normal 
inlining should cover it, assuming the processor has decent NaN support.


-Joe

Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input

[Joseph Darcy]

Hi Brian,

Lots good other than a few quibbles:

We usually use

/*
 *
 */

for long multi-line comments rather than

//
//
//

In the test update, we don't usually include mention of the bug id other 
than the @bug line.


Thanks,

-Joe

On 1/14/2014 11:56 AM, Brian Burkhalter wrote:

Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/.

Hopefully my verbose description of the very clever overflow test is accurate and 
understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the 
boundary values which split the range of the quantity "guard."

Thanks,

Brian

Re: RFR: JDK 9: 4891331: BigInteger a.multiply(a) should use squaring code

[Joseph Darcy]

Sound good to me :-)

Thanks

-Joe

On 12/13/2013 4:19 PM, Brian Burkhalter wrote:

The patch in questions was already approved in this thread

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-November/023611.html

so unless there are objections I shall push it to the new Java 9 repository.

Thanks,

Brian

Re: RFR: 8029944: (xs) Primitive Stream reduce method documentation pseudo code misidentifies apply method

[Joseph Darcy]

On 12/10/2013 5:05 PM, Mike Duigou wrote:

Hello all;

This is a documentation only fix for a bug reported by Michael McMahon. The reduce 
methods of the primitive streams classes currently reference an "apply" method 
rather than the appropriate applyAsInt, applyAsLong or applyAsDouble methods.

http://cr.openjdk.java.net/~mduigou/JDK-8029944/0/




Looks good; cheers,

-Joe

Re: JDK 8 RFR 8029514: java/math/BigInteger/BigIntegerTest.java failing since thresholds adjusted in 8022181

[Joseph Darcy]

Hi Brian,

I've taken a look at the change, but I don't understand why the problem 
wasn't surfaced before?


-Joe

On 12/4/2013 1:34 PM, Brian Burkhalter wrote:

Hello,

Issue:  https://bugs.openjdk.java.net/browse/JDK-8029514
Webrev: http://cr.openjdk.java.net/~bpb/8029514/webrev/

The problem causing this failure is that the method getLower() used by 
Karatsuba multiplication is expected to return an unsigned value but instead 
returns 'this' if the parameter 'n' is not greater than the int-length of the 
BigInteger on which it is invoked. For multiplications in which there is a 
negative factor this could result in an incorrect product being obtained.

The BigIntegerTest is also updated to reflect the modified thresholds (should 
have been in the patch for 8022181 but was not …).

Thanks,

Brian

Re: RFR: 8029489: StringJoiner spec for setEmptyValue() and length() is malformatted

[Joseph Darcy]

Hi Stuart,

Looks good; thanks,

-Joe

On 12/3/2013 5:40 PM, Stuart Marks wrote:

Hi all,

Please review the following small javadoc change. The StringJoiner doc 
for a couple methods uses "i.e." in the first sentence, which screws 
up the javadoc logic that pulls the first sentence into the Method 
Summary. This is an editorial change to fix this up; there is no 
actual specification change.


Thanks!

s'marks

# HG changeset patch
# User smarks
# Date 1386121046 28800
# Node ID 0bc91b9f6afd9b4dceea31ecd1036e367b365690
# Parent  75165f6c1c505ff43f7fd235a95b2e7955413b78
8029489: StringJoiner spec for setEmptyValue() and length() is 
malformatted

Reviewed-by: XXX

diff -r 75165f6c1c50 -r 0bc91b9f6afd 
src/share/classes/java/util/StringJoiner.java
--- a/src/share/classes/java/util/StringJoiner.javaTue Dec 03 
17:25:28 2013 -0800
+++ b/src/share/classes/java/util/StringJoiner.javaTue Dec 03 
17:37:26 2013 -0800

@@ -131,7 +131,7 @@
 /**
  * Sets the sequence of characters to be used when determining 
the string
  * representation of this {@code StringJoiner} and no elements 
have been
- * added yet, i.e. when it is empty.  A copy of the {@code 
emptyValue}
+ * added yet, that is, when it is empty.  A copy of the {@code 
emptyValue}
  * parameter is made for this purpose. Note that once an add 
method has been
  * called, the {@code StringJoiner} is no longer considered 
empty, even if

  * the element(s) added correspond to the empty {@code String}.
@@ -228,8 +228,8 @@
 }

 /**
- * The length of the {@code StringJoiner} value, i.e. the length of
- * {@code String} representation of the {@code StringJoiner}. 
Note that if

+ * Returns the length of the {@code String} representation
+ * of this {@code StringJoiner}. Note that if
  * no add methods have been called, then the length of the {@code 
String}
  * representation (either {@code prefix + suffix} or {@code 
emptyValue})

  * will be returned. The value should be equivalent to

Re: JDK 8 RFR 8029501: BigInteger division algorithm selection heuristic is incorrect

[Joseph Darcy]

Looks good Brian; thanks,

-Joe

On 12/3/2013 5:33 PM, Brian Burkhalter wrote:

Hello,

Issue:  https://bugs.openjdk.java.net/browse/JDK-8029501
Webrev: http://cr.openjdk.java.net/~bpb/8029501/webrev/

This patch would change the division algorithm selection heuristic as 
previously described in [1]. Many subsequent performance benchmark runs have 
determined that the threshold offset should be 40 if the division threshold 
itself is 80 [2]. With the existing algorithm selection heuristic, i.e., 
dividend and divisor int-length both above the tB-Z threshold, performance 
regressions of up to 100% will be observed when the int-lengths of the dividend 
and divisor exceed the B-Z threshold but the dividend int-length is less than 
approximately 40 more than the divisor int-length. The proposed patch fixes 
that problem.

Thanks,

Brian

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-November/023493.html
[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/023668.html

Re: JDK 8 RFR 8022181: Tune algorithm crossover thresholds in BigInteger

[Joseph Darcy]

Hi Brian,

The new thresholds look reasonable, as is planned follow-up tuning in JDK 9.

Thanks,

-Joe

On 12/2/2013 12:54 PM, Brian Burkhalter wrote:

Hello,

Issue:  https://bugs.openjdk.java.net/browse/JDK-8022181
Webrev: http://cr.openjdk.java.net/~bpb/8022181/webrev/

Based on numerous micro-benchmark test runs on various operating systems and 
architectures I would like to propose changing the BigInteger algorithm 
crossover thresholds as indicated in the webrev from which I excerpt here for 
convenience:

-private static final int KARATSUBA_THRESHOLD = 50;
+private static final int KARATSUBA_THRESHOLD = 80;
  
-private static final int TOOM_COOK_THRESHOLD = 75;

+private static final int TOOM_COOK_THRESHOLD = 240;
  
-private static final int KARATSUBA_SQUARE_THRESHOLD = 90;

+private static final int KARATSUBA_SQUARE_THRESHOLD = 128;

-private static final int TOOM_COOK_SQUARE_THRESHOLD = 140;
+private static final int TOOM_COOK_SQUARE_THRESHOLD = 216;
  
-static final int BURNIKEL_ZIEGLER_THRESHOLD = 50;

+static final int BURNIKEL_ZIEGLER_THRESHOLD = 80;

-private static final int SCHOENHAGE_BASE_CONVERSION_THRESHOLD = 8;
+private static final int SCHOENHAGE_BASE_CONVERSION_THRESHOLD = 20;

Each threshold is approximately the smallest value for which no statistically 
significant performance regression was observed with respect to the baseline. 
For the Toom-Cook algorithms, the baselines are the respective Karatsuba 
algorithms; for the other algorithms, the baselines are the respective 
algorithms previously implemented in BigInteger. Also, these new suggested 
thresholds were benchmarked against a performance baseline defined by the 
previous values of the respective thresholds.

With respect to the potential objection that the proposed thresholds are too 
high, please note that the values are subject to further refinement in the 
future. This issue https://bugs.openjdk.java.net/browse/JDK-8029425 has been 
filed as a reminder to continue to refine these values in the JDK 9 time frame 
although such fine tuning could as well occur in JDK 8 updates.

Thanks,

Brian

Re: RFR: 8027470: AnnotationSupport uses == rather than .equals to compare Class objects

[Joseph Darcy]

Hello,

Catching up on email, the specification of java.lang.Class does not 
explicitly promise that its notion of equality must be identity for all 
time. Therefore, while not required for today's implementations, I would 
prefer that new code we write in the JDK use equals rather than == when 
comparing classes.


Cheers,

-Joe

On 10/31/2013 3:24 AM, David Holmes wrote:

Hi Andreas,

On 31/10/2013 7:49 PM, Andreas Lundblad wrote:

Hi,

Please review the fix for JDK-8027470 below.

Description:
AnnotationSupport compared Class-instances using '==' where it should 
be using '.equals'. Fixed in this patch.


Class is final and does not override Object.equals therefore it is 
guaranteed to be based on identity. This change, while harmless, is 
unnecessary. Comparison of Class instances with == occurs throughout 
the JDK.


David
-


Link to web review:
http://cr.openjdk.java.net/~alundblad/8027470

Link to bug reports:
http://bugs.openjdk.java.net/browse/JDK-8027470

-- Andreas Lundblad

Re: Assorted annotation optimizations

[Joseph Darcy]

Joel,

If you haven't done so already, please file an rfe to capture Peter's 
suggestions.


Thanks,

-Joe

On 11/4/2013 7:29 AM, Joel Borggrén-Franck wrote:

Hi Peter,

As always, thanks for doing this!

On 2 nov 2013, at 22:58, Peter Levart  wrote:


Hi,

I propose a set of straightforward optimizations of annotations in the field of 
minimizing garbage creation and execution performance. Here's a webrev 
containing the changes:

http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationOptimizations/webrev.01/


Skimming this, I think most (or all) of this is good. However now is not the 
time to do it. This should go into 9 and 8u as soon as they are open for fixes.

FWIW I have also been toying with the idea of rewriting the storage of 
annotations in 9. IIRC you posted a patch with a very specific map for 
annotations. If we go down that road we should probably do a JEP.

cheers
/Joel

Re: Reported but unconfirmed JDK issue (Bug Id: 9002739)

[Joseph Darcy]

Hello,

The incident 9002739 became bug JDK-8014852 "(zipfs) 
ZipFileSystemProvider: newFileSystem URI format issue," which in turn 
was marked as a duplicate of JDK-7156873 "(zipfs) 
FileSystems.newFileSystem(uri, env) fails for uri with escaped octets.":


https://bugs.openjdk.java.net/browse/JDK-7156873

JDK-7156873 is fixed in JDK 8, 7u40, and 7u45.

HTH,

-Joe

On 11/13/2013 3:52 PM, Florian Brunner wrote:

Hi,

My name is Florian Brunner. This is my first E-Mail to this list. (So far I was 
only involved with the swing-dev and openjfx-dev mailing lists of OpenJDK.) So 
I hope, I'm sending my question to the right list.

Back in May I filed a JDK issue regarding the ZipFileSystemProvider (see the 
message below). But so far I cannot find the issue with the Bug Id: 9002739 on 
the Java Bug Database at http://bugs.sun.com.

Since a bug has been reported for Drombler FX (an Open Source project providing 
a modular Rich Client Platform for JavaFX based on OSGi and Maven (POM-first)), 
which probably is caused by this issue, I would like to work on this issue.

http://issues.drombler.org/14

Could someone from Oracle confirm this issue and make sure it appears in the 
Bug Database?

Thanks.

-Florian

-Original Message-
From: bug-report-daemon...@oracle.com [mailto:bug-report-daemon...@oracle.com]
Sent: Friday, May 17, 2013 7:09 PM
To: Brunner Florian (IT-SWE-CD2-T25)
Subject: Your Report (Bug ID: 9002739 ) - ZipFileSystemProvider: newFileSystem 
URI format issue

Dear Java Developer,
  
Thank you for reporting this issue.


We have determined that this report is a new bug and have entered the bug into 
our bug tracking system under Bug Id: 9002739. You can look for related issues 
on the Java Bug Database at http://bugs.sun.com.

We will try to process all newly posted bugs in a timely manner, but we make no 
promises about the amount of time in which a bug will be fixed. If you just 
reported a bug that could have a major impact on your project, consider using 
one of the technical support offerings available at Oracle Support.

Thanks again for your submission!

Regards,
Java Developer Support

Re: RFR: 8028027: serialver should emit declaration with the 'private' modifier

[Joseph Darcy]

Approved!!

-Joe

On 11/7/2013 7:02 PM, Stuart Marks wrote:

Hi all,

Please review this quick one-liner to change the serialver tool so 
that it emits a serialVersionUID declaration with the 'private' 
modifier, which comports with the recommendation in the 
java.io.Serializable page.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8028027

Patch appended below.

Thanks,

s'marks


--


# HG changeset patch
# User smarks
# Date 1383868023 28800
# Node ID 1e1088bfea50d7d3cc7cfdce2b0085b7adc6a180
# Parent  f18b60bd22a1be988d329960c46d504f4b75000f
8028027: serialver should emit declaration with the 'private' modifier
Reviewed-by: XXX

diff -r f18b60bd22a1 -r 1e1088bfea50 
src/share/classes/sun/tools/serialver/SerialVer.java
--- a/src/share/classes/sun/tools/serialver/SerialVer.javaThu Nov 
07 15:45:21 2013 -0800
+++ b/src/share/classes/sun/tools/serialver/SerialVer.javaThu Nov 
07 15:47:03 2013 -0800

@@ -211,7 +211,7 @@
 Class cl = Class.forName(classname, false, loader);
 ObjectStreamClass desc = ObjectStreamClass.lookup(cl);
 if (desc != null) {
-return "static final long serialVersionUID = " +
+return "private static final long serialVersionUID = " +
 desc.getSerialVersionUID() + "L;";
 } else {
 return null;

Re: RFR: 8016725: TEST_BUG: java/lang/reflect/Method/DefaultMethodModeling.java failing intermittently

[Joseph Darcy]

Hi Andreas,

Approved; thanks,

-Joe

On 10/29/2013 3:26 AM, Andreas Lundblad wrote:

Hi,

Please review the fix for JDK-8016725 below.

Description:
DefaultMethodModeling.java and Equals.java in
jdk/test/java/lang/reflect/Method interfered with each other since
both tests defines a class named 'A'.

In this patch DefaultMethodModeling.java is moved to its own
subdirectory.


Link to web review:
http://cr.openjdk.java.net/~alundblad/8016725/webrev.00

Link to bug report:
https://bugs.openjdk.java.net/browse/JDK-8016725

-- Andreas Lundblad

Re: JDK 8 RFC 4891331: BigInteger a.multiply(a) should use squaring code

[Joseph Darcy]

Hi Brian,

On 10/17/2013 12:26 PM, Brian Burkhalter wrote:

This post concerns this issue:

https://bugs.openjdk.java.net/browse/JDK-4891331

I performed some tests using JMH [1] on Mac OS X [2] and Windows 7 [3]. The 
tests were equivalent to calling multiply() with argument == this for bit 
lengths from 32 through 448 without and with this patch applied:

--- a/src/share/classes/java/math/BigInteger.java   Thu Oct 17 11:34:01 
2013 -0400
+++ b/src/share/classes/java/math/BigInteger.java   Thu Oct 17 11:45:42 
2013 -0700
@@ -1374,6 +1374,10 @@
  if (val.signum == 0 || signum == 0)
  return ZERO;
  
+if (val == this) {

+return square();
+}
+
  int xlen = mag.length;
  int ylen = val.mag.length;

A table of the ratios of throughput with the patch applied to throughput 
without the patch applied is here:



Can you test the performance of a patch with:

if (val == this and mag.length > 8)

Do you have any measurements of the multiply performance of values that 
are not ==? (Making sure there is no general regression.)


Thanks,

-Joe

Re: Review request for JDK-8026011: j.l.r.MalformedParametersException introduces doclint warnings

[Joseph Darcy]

Hi Eric,

Looks fine; thanks,

-Joe

On 10/10/2013 3:48 PM, Eric McCorkle wrote:

Hello,

Please review this simple patch that adds javadoc comments to
MalformedParametersException to quiet doclint warnings.

Note this patch involves no code changes.

The bug report is here:
https://bugs.openjdk.java.net/browse/JDK-8026011

The webrev is here:
http://cr.openjdk.java.net/~emc/8026011/

Thanks,
Eric

Re: RFR: 7044282: (reflect) Class.forName and Array.newInstance are inconsistent regarding multidimensional arrays

[Joseph Darcy]

Hi Joel,

The code changes look fine, but I'd like to see some refactoring to the 
tests. In particular, please put the logic in


  81 try {
  82 Class c256 = Class.forName(name256);
  83 error++;
  84 System.err.println("ERROR: could create " + c256);
  85 } catch (ClassNotFoundException e) {
  86 ;// ok
  87 }

into a method that can be called like failingForName(name, clazz) (or 
whatever is appropriate).


Thanks,

-Joe

On 10/9/2013 11:33 AM, Joel Borggren-Franck wrote:

Hi

Please review this spec update and test for getting array classes and
instances of more dimensions than the class file can express or the VM
can handle.

Array.newInstance have a test for arrays of more dimensions than 255,
this patch adds a test for Class.forName as well.

Also the javadoc for Array.newInstance are clarified.

Bug: https://bugs.openjdk.java.net/browse/JDK-7044282
Webrev: http://cr.openjdk.java.net/~jfranck/7044282/webrev.00/

cheers
/Joel

Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation

[Joseph Darcy]

I'll pushed with the terminal op reordering you've suggested; thanks for 
the reviews,


-Joe

On 10/9/2013 6:17 PM, Mike Duigou wrote:

For consistency I would move the "this is a terminal operation" paragraph to 
just before the @apiNote. I would suggest after the @apiNote but there's no mechanism for 
ending a javadoc tag other than starting another tag and we don't want the terminal text 
in the apiNote.

Other than that it looks correct.

Mike

On Oct 9 2013, at 17:57 , Joe Darcy wrote:


On 10/09/2013 04:47 PM, Mike Duigou wrote:

Big improvement. This looks like the right direction. DoubleStream.average() 
needs the same treatment for completeness.

Thanks Mike. Next iteration incorporating this feedback (and some other 
off-list feedback):

   http://cr.openjdk.java.net/~darcy/8024354.2

-Joe

Re: RFC 6910473: BigInteger negative bit length, value range, and future prospects

[Joseph Darcy]

Without comments on the contents of the patch, a change in documented 
behavior would require a ccc request.


Cheers,

-Joe

On 10/3/2013 5:58 PM, Brian Burkhalter wrote:

I have reviewed this proposed change a couple of times in its current form and 
it looks good to me.

It would be good to see some comments about the general concept from BigInt 
cognoscenti, and from (a) Reviewer(s) as concerns the @implNote addition as 
well as the new ArithmeticExceptions added at several points in the javadoc. 
With respect to these latter, in the event the patch were to be approved, would 
a CCC request be in order?

Brian

On Oct 1, 2013, at 7:50 PM, cowwoc wrote:


Sounds good. Thanks for the clarification.

Gili

On 01/10/2013 9:25 PM, Dmitry Nadezhin wrote:

I see that I misused the word "to clamp" in this discussion.
I guess that addition with "clumping" means:
return x + y < MIN_VALUE ? MIN_VALUE : x + y > MAX_VALUE ? MAX_VALUE : x +
y;

The patch actually throws ArithmeticException on overflow:
if (x + y < MIN_VALUE || x + y > MAX_VALUE) throw new ArithmetiException();
else return x + y;

The word "clamp" appeared in the discussion only.
Comments in the patch don't contain it. They say:

BigInteger constructors and operations throw {@code
ArithmeticException} whenthe result is out of the supported range.

On Tue, Oct 1, 2013 at 11:45 PM, cowwoc  wrote:


 I prefer throwing exceptions on unusual conditions (e.g. overflow) and
letting the user clamp the value if they so wish. Clamping will lead to
unexpected behavior once values fall outside this range. Yes, it will be
documented, but I daresay most applications won't ever check for it and
produce incorrect results as a consequence.

Gili


On 01/10/2013 2:19 PM, Dmitry Nadezhin wrote:


Dear BigInteger experts,
Do you have comments to my previous message ?
http://mail.openjdk.java.net/**pipermail/core-libs-dev/2013-**
September/021264.html

Re: 8008662: Add @jdk.Exported to JDK-specific/exported APIs

[Joseph Darcy]

Hello,

I skimmed the patch and it looked fine.

More generally, we want every package and top-level class in the 
com.sun.* namespace to be either explicitly exported or not.


Cheers,

-Joe

On 10/6/2013 1:03 PM, Alan Bateman wrote:


As a follow-up to Joe Darcy's rename of jdk.Supported to jdk.Exported, 
I'd like to have another attempt at adding the annotation to a number 
of JDK specific APIs that are long standing exported, documented and 
supported APIs. Specifically, the following APIs:


- Java Debug Interface (com.sun.jdi)
- Attach API (com.sun.tools.attach)
- SCTP API (com.sun.nio.sctp)
- HTTP server API (com.sun.net.httpserver)
- Management extensions (com.sun.management)
- JConsole Plugin API (com.sun.tools.jconsole)
- JDK-specific API to JAAS (com.sun.security.auth)
- JDK-specific JGSS API (com.sun.security.jgss)

(The javadoc for each of these APIs is currently generated in the build)

The webrev with the proposed update is here:
  http://cr.openjdk.java.net/~alanb/8008662/webrev.02/

As per the original webrev, I've added package-info.java to a number 
of packages that didn't have any description. In a few cases, I've had 
to rename the legacy package.html to package-info.java.


For the review then the intention is that @jdk.Exported be added to 
the package-info and all public/protected types in these APIs. The 
only exceptions are two cases where I've added @jdk.Exported(false), 
specifically:


- com.sun.management.OSMBeanFactory as it clearly documents to stay away
- com.sun.security.auth.callback.DialogCallbackHandler as it for the 
chop (see JEP 162)


Thanks,

Alan.

Re: Question about JDK-8023087

[Joseph Darcy]

Enum constructors (as compiled by javac) have synthetic parameters; 
constructors of nested classes [1] can have either implicit or synthetic 
parameters.


HTH,

-Joe

[1] https://blogs.oracle.com/darcy/entry/nested_inner_member_and_top

On 9/30/2013 8:25 PM, Eric Wang wrote:

Including the corelibs-dev team, so someone may help me.

Thanks,
Eric
On 2013/10/1 10:39, Eric Wang wrote:

Hi Alan,

I'm looking at the bug 
https://bugs.openjdk.java.net/browse/JDK-8023087 which is filed for 
CCC changes https://bugs.openjdk.java.net/browse/JDK-8007405.


There're 2 simple APIs isImplicit() and isSynthetic() changed in 
java.lang.reflect.Parameter, which are not covered by a test.


However, based on the API desciptions below, I have no idea how to 
test them as I can't find such case, Can you please give some 
suggestion or some examples?


/**
 * Returns true if this parameter is implicitly declared in source 
code; returns ode false otherwise.

 *
 * @return true if and only if this parameter is implicitly declared 
as defined by The Java Language Specification.

 */
public boolean isImplicit() {return 
Modifier.isMandated(getModifiers());}


/**
 * Returns true if this parameter is neither implicitly nor 
explicitly declared in source code; returns false otherwise.

 *
 * @jls 13.1 The Form of a Binary
 * @return true if and only if this parameter is a synthetic 
construct as defined by The Java Language Specification.

 */
 public boolean isSynthetic() {return 
Modifier.isSynthetic(getModifiers());}


Thanks,
Eric
On 2013/10/1 9:05, Michel Trudeau wrote:

Eric,

Any update on this bug ?  We really need to close this down as soon 
as possible.


https://bugs.openjdk.java.net/browse/JDK-8023087

Thanks,
Michel

On Sep 19, 2013, at 10:18 AM, Michel Trudeau 
 wrote:


Hopefully the right Eric this time around.

Michel

On Sep 19, 2013, at 10:12 AM, eric wang  wrote:

wrong Eric Wang I assume.

Eric
On 9/19/2013 8:58 AM, Michel Trudeau wrote:

Eric,

Any update on this ?  We really need to get those tests in ASAP.

Thanks,
Michel

On Sep 12, 2013, at 10:40 AM, Michel Trudeau 
 wrote:


Eric,

This bug got assigned to you yesterday.   Not sure if this is 
really a P4, sounds like a P3 since we need those tests.


Is this a top priority for you ?   I would really like to get this 
done sooner than later.


https://bugs.openjdk.java.net/browse/JDK-8023087

Thanks,
Michel

Re: RFR: 7057785 : (xs) Add note to hashCode() that support for self referential is optional

[Joseph Darcy]

Looks fine; cheers,

-Joe

On 9/16/2013 3:49 PM, Mike Duigou wrote:

Ping!

(still need a reviewer on this)

Mike

On Sep 4 2013, at 11:44 , Mike Duigou wrote:


Hello all;

I have updated the proposed changeset for this issue. I have moved the note to 
the interface documentation for Collection and Map and made it more general:


Some collection operations which perform recursive traversal of the
collection may fail with an exception for self-referential instances where
the collection directly or indirectly contains itself. This includes the
{@code clone()}, {@code equals()}, {@code hashCode()} and {@code toString()}
methods. Implementations may optionally handle the self-referential scenario,
however most current implementations do not do so.

The webrev is at:

http://cr.openjdk.java.net/~mduigou/JDK-7057785/1/webrev/

Mike

On Aug 27 2013, at 19:06 , Mike Duigou wrote:


Hello all;

Fairly frequently it is reported that various Collection/Map implementations of 
hashCode() fail when the instance directly or indirectly contains itself. For a variety 
of reasons, mostly performance and resource related, most implementations choose not to 
support calculation of hash codes for self-referential collections. This is not likely to 
change. So to reduce confusion and "bug" reports I am proposing a non-normative 
@apiNote be added to Collection and HashMap. The text of the proposed note is:


Support for calculation of hash code by self referential {Collection|Map}s 
(they either directly or indirectly contain themselves) is optional. Few 
Collection implementations support calculation of hash code for self 
referential instances.


http://cr.openjdk.java.net/~mduigou/JDK-7057785/0/webrev/

Cheers,

Mike

Re: Please review two corrections for java.time

[Joseph Darcy]

Looks fine; cheers,

-Joe

On 9/13/2013 12:07 PM, roger riggs wrote:
Ping, Reviewer needed for these minor updates, the test is now more 
robust thanks to Peter's nudging.


http://cr.openjdk.java.net/~rriggs/webrev-localtime-now-8023639/

Please review for two corrections:

-  The java/time/tck/java/time/TCKLocalTime test failed on a slow 
runtime due to -Xcomp;
the test should be more lenient.   The test is not appropriate for 
a conformance test
and is moved to java/time/test/java/time/TestLocalTime and is 
updated to be

more robust in the face of DST changes.

- The javadoc for the JapaneseEra.MEIJI era should indicate the start 
date is 1868-01-01
  to be consistent with java.util.Calendar.  Note that java.time does 
not permit dates before Meiji 6

  to be created since the calendar is not clearly defined until then.

Thanks, Roger

Re: java.lang.reflect.Parameter comments

[Joseph Darcy]

Hello,

Sending along some responses to these questions from Alex:

On 8/25/2013 7:03 AM, Kasper Nielsen wrote:

Hi,

just 2 short questions/commons on java.lang.reflect.Parameter

1)
I was wondering if there is any reason for java.lang.reflect.Parameter not
to expose the index field?


"Not sure what you mean by the "index field", but if you mean the 
name_index item in the MethodParameters attribute, then it is possible 
to know whether it's zero by calling Parameter#isNamePresent."




2)
Also, while Parameter.getParameterizedType() might be a better name than
getGenericType().
I find it kind of annoying that it is called Field.getGenericType() but
Parameter.getParameterizedType(). Lets just have the same name for
practically the same functionality?



"Yes and no. First, the historic use of getGenericXXX in 
java.lang.reflect is wrong (it should be getParameterizedXXX) but it 
can't be changed now. The Language Model API in javax.lang.model.** is 
much better with terminology. Second, when we add methods to existing 
java.lang.reflect types, we hold our noses and use "Generic" for 
consistency, e.g., the new AnnotatedArrayType interface for type 
annotations has a method getAnnotatedGenericComponentType that is 
consistent with the getGenericComponentType method in GenericArrayType. 
Third, when we add new types to java.lang.reflect, we use the correct 
terminology, hence Parameter#getParameterizedType."


HTH,

-Joe

Re: @Supported design issues

[Joseph Darcy]

On 9/10/2013 10:08 AM, David M. Lloyd wrote:

On 09/10/2013 11:54 AM, Mandy Chung wrote:

On 9/10/13 9:47 AM, Joe Darcy wrote:


On 9/10/2013 6:28 AM, Alan Bateman wrote:

On 06/09/2013 04:23, mark.reinh...@oracle.com wrote:

:
Well, looking ahead to when the platform will be composed of modules,
those modules will declare that they "export" some API elements, but
not others.  An @Exported annotation would help get people used to
the expected future terminology.

@Exported is quite good, and consistent with where this is likely 
going.


Joe - what would you think of just running with this? I'm anxious
that we decide on this soon so that we don't run out of time in jdk8.



I don't object to using @Exported.


I like @Exported as well.


If we're framing it in terms of modules, I think it would make more 
sense to have exporting be default and "hidden" be opt-in.


And, while we're at it, "hidden" really ought to apply at a package 
level, not a class level.


In other words: don't make this about modularity.


To bring in some of the initial context, this feature is about 
documenting and formalizing the historically unclear 
exported-ness/supported-ness of types in the com.sun.* packages. Some 
com.sun.* types are intended to be used outside of the JDK while others 
are not.


To bring clarity to this situation, I'd like to see each type and 
package in com.sun.* either have an explicit @Exported(true) XOR 
@Exported(false) annotation applied to it. This make a clear statement 
around the intentions of the type and will allow better tooling to be 
written.


-Joe

Re: @Supported design issues

[Joseph Darcy]

On 9/5/2013 2:20 AM, Alan Bateman wrote:

On 20/03/2013 01:32, Joseph Darcy wrote:


Following up in the same thread, the JEP for this work is now 
available for your reading pleasure at:


JEP 179: Document JDK API Support and Stability
http://openjdk.java.net/jeps/179
Joe - do you want to reboot this discussion? With the deadline for API 
changes looming then it would be good to get agreement on whether we 
are going to do anything on this. I still have the patch to add this 
to the APIs that we generate javadoc for in the build - this is the 
same set of APIs that I understand to be stable APIs and okay for 
applications/libraries to make direct use of.


Given the previous discussion then getting agreement on whether this 
is a boolean or something more seems important. Clearly there are 
corner cases (a few of these came up in the original discussion) but a 
simple label to convey that an API is stable seems a good start. The 
other problematic issue was the naming, clearly "Supported" results in 
too many questions, "by who?" in particular. Have you considered 
alternative names? I realize this is open to bikeshedding. Personally 
I wouldn't have a problem with jdk.Stable if appropriately defined.




IMO, the high order goal here should be getting the "is this API okay to 
use" information encoded into the source code and class files. Given 
that you've already compiled that information, I think there is great 
value in going forward with this effort for JDK 8 even given the 
relatively late point in the schedule.


Perhaps instead of "Supported", the adjective "Sanctioned" better 
conveys what is intended: this API is explicitly part of the JDK's 
contract and fine to use.


I'm open to other suggestions too.

Cheers,

-Joe

Re: RFR: 8023447: change specification to allow RMI activation to be optional

[Joseph Darcy]

Looks fine; cheers,

-Joe

On 9/5/2013 3:46 PM, Stuart Marks wrote:

Hi all,

Please review this specification-only change to allow RMI activation 
to be optional. RMI activation, unlike the rest of RMI, pretty much 
requires the ability to fork processes at will. This causes 
difficulties in certain situations, such as in small embedded 
configurations. Activation is typically unnecessary in such 
environments, hence it makes sense for it to be optional.


Essentially the change is the addition of a small paragraph to the 
package doc for java.rmi.activation, and adding spec for throwing 
UnsupportedOperationException to a bunch of methods in this package.



Bug report:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8023447

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8023447/webrev.5/

Specdiff:

http://cr.openjdk.java.net/~smarks/reviews/8023447/specdiff.5/overview-summary.html 



Thanks,

s'marks

Re: Java 8 RFR 8010430: Math.round has surprising behavior for odd values of ulp 1

[Joseph Darcy]

Hello,

On 8/23/2013 1:36 PM, Guy Steele wrote:

The specification of java.lang.Math.round in the first edition of the
Java Language Specification is quite clear:

public static int round(float a)
   The result is rounded to an integer by adding 1/2, taking the floor of
   the result, and casting the result to type int.
   In other words, the result is equal to the value of the expression
(int)Math.floor(a + 0.5f)

and a similar statement for the case where the type of the argument is double.

This does not correspond to "rounding away from zero" as in IEEE754.

The phrase "ties rounding up" entered the Java documentation later on
as a (perhaps unfortunately worded) shorthand for the original specification.


To give some context, the original specification was changed in JDK 7 
under bug


JDK-6430675 Math.round has surprising behavior for 0x1.fp-2
http://bugs.sun.com/view_bug.do?bug_id=6430675

At the time, it was making the rounds that Math.round gave a "wrong" 
answer for an input value equal to the largest floating-point value less 
than 0.5: Math.round(0.49994) returned 1 rather than 0. The 
result is wrong in terms of being unexpected; it was the result mandated 
by the operational definition of the specification.


I addressed JDK-6430675 by changing the implementation and removing the 
operational definition of Math.round. While I thought I had ruled out 
unexpected round-off behavior at the other end of the range, mea culpa, 
I did not account for the issues reported in 8010430.


-Joe



--Guy Steele


On Aug 23, 2013, at 4:24 PM, Dmitry Nadezhin  wrote:


The specification of java.lang.Math.round() says
* Returns the closest {@code int} to the argument, with ties
 * rounding up.

It is not clarified what is "ties rounding up".
I guess that it should correspond to the direction "roundTiesToAway" of
IEEE 754-2008
and to the java.math.RoundingMode.HALF_UP .

They round
+0.5 -> +1
-0.5 -> -1

The current implementation of java.lang.Math.round() rounds
+0.5 -> +1
-0.5 -> 0

"ties rounding up" should match IEEE754 standard and other JDK math class,
shouldn't it ?


On Fri, Aug 23, 2013 at 10:32 PM, Brian Burkhalter <
brian.burkhal...@oracle.com> wrote:


This message follows the RFC
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-August/019560.htmlposted
 on August 2.

The issue is http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010430.

The proposed patch http://cr.openjdk.java.net/~bpb/8010430/ has the
effect of option (A) in the aforementioned RFC.

Thanks,

Brian

Re: RFR: 8022343: j.l.Class.getAnnotatedSuperclass() doesn't return null in some cases

[Joseph Darcy]

Hi Joel,

The new version is better, but for the testing in question I would 
prefer to see something even simpler like:


public static void main(String[] args) throws Exception {
int failed = 0;
Class[] testData = {/* list of class literals*/}
for (Class toTest: testData) {
Object res = toTest.getAnnotatedSuperclass();

if (res != null) {
failed++;
System.out.println(toTest + ".getAnnotatedSuperclass() 
returns: "

+ res + ", was non-null");
}
}

if (failed != 0)
throw new RuntimeException("Test failed, check log for 
details");

}


-Joe

On 8/21/2013 5:04 AM, Joel Borggrén-Franck wrote:

Hi Joe, Paul

I rewrote the test in Paul's style without using testNG.

http://cr.openjdk.java.net/~jfranck/8022343/webrev.01/

Please review.

cheers
/Joel

On 2013-08-19, Joe Darcy wrote:

Hi Joel,

I agree the code looks fine.

However, I concur with the general sentiment of Paul test advice
without advocating using testng for this task.

A loop over a Class[] initialized with the kinds of values of
interest would seem to be better structured to me and allow for
better exception handing, etc.

-Joe

On 08/19/2013 01:34 AM, Paul Sandoz wrote:

Hi Joel,

The fix looks OK.

Not suggesting you do the following, unless you really want to, but the test is 
an example of where TestNG data providers are useful, since all cases will be 
tested and reported for pass or failure, rather than in this case the first 
failure will cause other checks (if any) not to be tested and in addition will 
not report which check failed (one needs to look at the stack trace).

Not tested:

   @DataProvider(name = "class")
   private static Object[][] getClasses() {
   // Using the stream API because i can :-)
   // Arguably simpler in this case to use new Object[][] { {} }
   return Stream.of(
 Object.class,
 If.class,
 Object[].class,
 void.class,
 int.class).
   map(e -> new Object[] { e }).
   toArray(Object[][]::new);
   }

   @Test(dataProvider = "class")
   public void testAnnotatedSuperClassIsNull(Class c) {
   assertNull(c.getAnnotatedSuperclass())
   }

Paul.

On Aug 16, 2013, at 2:17 PM, Joel Borggren-Franck  
wrote:


Hi

Please review this small fix for a type annotation reflection issue.

The javadoc spec for Class.getAnnotatedSuperclass says:

* If this Class represents either the Object class, an interface type, an
* array type, a primitive type, or void, the return value is null.

The patch fixes this.

Webrev at: http://cr.openjdk.java.net/~jfranck/8022343/webrew.00/

Patch also included it at the end of this mail.

cheers
/Joel



diff -r b07b19182e40 src/share/classes/java/lang/Class.java
--- a/src/share/classes/java/lang/Class.javaThu Aug 15 15:04:59 2013 +0100
+++ b/src/share/classes/java/lang/Class.javaFri Aug 16 13:20:31 2013 +0200
@@ -3338,8 +3338,16 @@
  * @since 1.8
  */
 public AnnotatedType getAnnotatedSuperclass() {
- return 
TypeAnnotationParser.buildAnnotatedSuperclass(getRawTypeAnnotations(), 
getConstantPool(), this);
-}
+if(this == Object.class ||
+isInterface() ||
+isArray() ||
+isPrimitive() ||
+this == Void.TYPE) {
+return null;
+}
+
+return 
TypeAnnotationParser.buildAnnotatedSuperclass(getRawTypeAnnotations(), 
getConstantPool(), this);
+}

 /**
  * Returns an array of AnnotatedType objects that represent the use of 
types to
diff -r b07b19182e40 test/java/lang/annotation/TypeAnnotationReflection.java
--- a/test/java/lang/annotation/TypeAnnotationReflection.java   Thu Aug 15 
15:04:59 2013 +0100
+++ b/test/java/lang/annotation/TypeAnnotationReflection.java   Fri Aug 16 
13:20:31 2013 +0200
@@ -23,7 +23,7 @@

/*
  * @test
- * @bug 8004698 8007073
+ * @bug 8004698 8007073 8022343
  * @summary Unit test for type annotations
  */

@@ -58,7 +58,7 @@
 }

 private static void testSuper() throws Exception {
-check(Object.class.getAnnotatedSuperclass().getAnnotations().length == 
0);
+check(Object.class.getAnnotatedSuperclass() == null);
 check(Class.class.getAnnotatedSuperclass().getAnnotations().length == 
0);

 AnnotatedType a;
diff -r b07b19182e40 
test/java/lang/annotation/typeAnnotations/GetAnnotatedSuperclass.java
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/test/java/lang/annotation/typeAnnotations/GetAnnotatedSuperclass.java 
Fri Aug 16 13:20:31 2013 +0200
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribu

Re: code review request: JDK-8015780: java/lang/reflect/Method/GenericStringTest.java failing

[Joseph Darcy]

Hi Vicente,

Looks fine; approved to go back.

Thanks,

-Joe

On 8/6/2013 6:59 AM, Vicente-Arturo Romero-Zaldivar wrote:

Hello,

Please review this patch, which updates test 
jdk/test/java/lang/reflect/Method/GenericStringTest.java for it to 
pass after changes introduced to generation of bridge methods.


Now if a bridge method is generated, all annotations present in the 
related "standard" method are copied to the bridge method.


Part of the job of this test is to compare the method generic string 
against the one provided in the ExpectedGenericString annotation. The 
fix is to add another element to the same annotation in case a bridge 
method is generated. Later it's needed to determine which element's 
value we need to compare with.


The webrev is here:
http://cr.openjdk.java.net/~vromero/8015780/webrev.00/

The bug report is here:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8015780

Thanks,
Vicente.

Re: Code Review Request: More tests for 7184826: (reflect) Add support for Project Lambda concepts in core reflection

[Joseph Darcy]

Hello,

Sorry for the repeated review delays.

This looks fine to go back.

Thanks,

-Joe

On 7/22/2013 6:27 AM, Joel Borggren-Franck wrote:

Hi Amy,

I'm happy with the current iteration. I'll help you find an official
reviewer.

cheers
/Joel

On 2013-07-22, Amy Lu wrote:

Thank you Joel for all the valuable feedback.

Test have been refactored and here comes the updated version:
https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.01/index.html

Thanks,
Amy


On 7/10/13 10:17 PM, Joel Borggren-Franck wrote:

Hi Amy, Tristan,

I'm not a Reviewer kind of reviewer, but I've started to look at the
code and can sponsor this.

Some comments on test/java/lang/reflect/Method/invoke/DefaultStaticTest.java:

As there are a lot of non-public top-level classes perhaps this test
should be in it own directory.

It is very hard to read the data table:

292 {interface1.class, 1, 1, new Object1(), new Object[]{},
293 new Object[]{}, "interface1", null},

I believe you should move the methodsNum constant and the declMethods
num constant to an annotation on the interface/class in question. For
example:

@MyTestData(numMethods=1, declMethods=1)
41 interface interface1 {
42 @DefaultMethod(isDefault = true)
43 @StaticMethod(isStatic = false)
44 @ReturnValue(value = "interface1.defaultMethod")
45 default String defaultMethod(){ return "interface1.defaultMethod"; };
46 }

That way it is much easier to inspect that the constants are right.

The same can probably be done with the return values encoded.

Instead of all these "new Objects[] {}" can't you create a field,

Object [] EMPTY_PARAMETERS = new Object() {}

and reuse it?

That way it will be much easier to verify that the encoded test data is
correct.

I'll comment on the other tests shortly.

cheers
/Joel

On 2013-06-13, Amy Lu wrote:

This has been pending on review for long ... Please help to review.
I also need a sponsor for this.

Thank you very much.

/Amy

On 5/23/13 10:48 PM, Amy Lu wrote:

Please help to review:

More tests for 7184826: (reflect) Add support for Project Lambda
concepts in core reflection

This includes:
1. improvement for existing tests with more situations
2. Test for invoking interface default/static method by j.l.r.Method
3. Test for invoking interface default/static method by MethodHandle

https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.00/index.html


Thanks,
Amy

Re: Classes on the stack trace (was: getElementClass/StackTraceElement, was: @CallerSensitive public API, was: sun.reflect.Reflection.getCallerClass)

[Joseph Darcy]

On 7/29/2013 9:31 PM, Alan Bateman wrote:

On 29/07/2013 19:17, David M. Lloyd wrote:


Your phrasing makes me think I missed something: is the 
Reflection.getCallerClass() method being removed due to some 
technical issue that it can only be somehow emulated as a 
workaround?  Or is it just a general cleanup effort?


The sun.reflect.Reflection.getCallerClass(int) method was removed as 
part of JEP 176 [1].


-Alan.

[1] http://openjdk.java.net/jeps/176



And as a sun.* method, this was officially outside of the exported 
interface of the JDK and not something that should be expected to work 
if called from outside of the JDK:


http://www.oracle.com/technetwork/java/faq-sun-packages-142232.html

-Joe

Re: RFR: 8021601 : (xxs) Add unit test for PriorityQueue(Comparator) constructor

[Joseph Darcy]

Looks good Mike; cheers,

-Joe

On 7/26/2013 5:06 PM, Mike Duigou wrote:

Hello all;

JDK-6799426 (http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a6cbb9808e4b) was 
pushed without a unit test. (Always recheck for unit tests when breathing life 
back into a stale old patch). A unit test is needed. This change adds an 
additional case to an existing test to exercise the new constructor.

http://cr.openjdk.java.net/~mduigou/JDK-8021601/0/webrev/

I have also tweaked the constructor documentation per Paul Benedict's 
suggestion.

Thanks,

Mike

Re: RFR JDK 8 javac lint cleanup of java.lang.ref

[Joseph Darcy]

An update, I played around with the declarations a bit more, but wasn't 
about to find something workable so I pushed the already-reviewed version.


If someone else wants to take a crack at improving the generics, I think 
that would be a fine refactoring.


Thanks,

-Joe

On 7/25/2013 2:16 PM, Joe Darcy wrote:

On 07/25/2013 01:58 PM, Alan Bateman wrote:

On 25/07/2013 13:33, Joe Darcy wrote:

Hello,

Please review these changes to remove the javac lint warnings from 
the java.lang.ref package:


8021429 Fix lint warnings in java.lang.ref
http://cr.openjdk.java.net/~darcy/8021429.0/

Care was taken to not change any signatures of public API elements. 
After doing a clean build, all the java.lang.ref regression tests 
pass. Full patch below.


Thanks,

-Joe
This looks okay to me, just wondering Reference.next needs to be a 
raw type (you may have tried possible solutions to this already).


After some amount of examination, I didn't find a way to fix this. The 
description of the protocol around that field is


/* When active:   NULL
 * pending:   this
 *Enqueued:   next reference in queue (or this if last)
 *Inactive:   this
 */

None of

Reference
Reference
Reference
Reference

work as the declared type of the field given other uses of the field 
in the package.




Minor comment on Finalizer is that the comment on the class being 
package-private is now misaligned.





I'll fix that before I push.

Thanks all for the quick reviews.

-Joe

Re: Java 8 RFR 6476168: (fmt) Inconsistency formatting subnormal doubles with hexadecimal conversion

[Joseph Darcy]

Hi Brian,

Almost there! A few additional comments.

On 7/22/2013 4:47 PM, Brian Burkhalter wrote:

An updated webrev is in the same location: 
http://cr.openjdk.java.net/~bpb/6476168/.

On Jul 19, 2013, at 5:38 PM, Joseph Darcy wrote:


A spec quibble "decimal separator" isn't really the appropriate term for the 
hex formatting.

I've changed this in the verbiage on lines 613 and 1376 of Formatter. The other usages of 
"decimal separator" are unchanged.


I think some more test cases and needed:

* Subnormal result rounding up to normal range under reduced precision. 
Something like nextDown(Double.MIN_NORMAL) (a subnormal value) rounded to 
between 1 and 11 digits of precision.


The changes in that block include

1353 // 6476168: subnormal formatting and precision

It is not customary, and usually not desirable, to include the bug id. 
Additionally, the new block of test cases tests for subnormal as well as 
underflow the comment is not accurate.


It is not necessary to write

Double.parseDouble("0x0.123p-1022")

you can just write

0x0.123p-1022

using the hexadecimal notation for a floating-point literal.

I'd like to see a few more test cases that probe at boundaries, such as 
Double.MAX_VALUE rounding to 9, 6, 2, 1, digits of precision.


There are also cases of interest to make sure the round to nearest even 
rounding policy is being used. If a value like 0.123 is 
rounded to three digits, the value of  will determine 
whether or not an increment occurs.




Added as Basic-X line 1360.


* Double.MAX_VALUE rounded to fewer than 12 digits. Offhand, I'm not sure what 
the implementation will do here; returning infinity or a hex string with an 
extra big exponent are both defensible.

Added as Basic-X line 1361. The returned value for %1.9a is 0x1.0p1024.


Of the two options, that value is preferable.

Thanks,

-Joe




Thanks,

Brian

Re: Java 8 RFR 6476168: (fmt) Inconsistency formatting subnormal doubles with hexadecimal conversion

[Joseph Darcy]

On 7/18/2013 3:20 PM, Brian Burkhalter wrote:

Hi Joe,

On Jul 17, 2013, at 10:31 PM, Joe Darcy wrote:


In the javadoc change, is there a reason for

[1, 12],

rather than just

[1, 12],

?

Not really.


The update should discuss how normal (that is non-subnormal) values are handled 
with reduced precision.

The change should include tests of the newly specified behavior.

The requested changes have been made, tested, and incorporated into the webrev

http://cr.openjdk.java.net/~bpb/6476168/




Hello,

A spec quibble "decimal separator" isn't really the appropriate term for 
the hex formatting.


I think some more test cases and needed:

* Subnormal result rounding up to normal range under reduced precision. 
Something like nextDown(Double.MIN_NORMAL) (a subnormal value) rounded 
to between 1 and 11 digits of precision.


* Double.MAX_VALUE rounded to fewer than 12 digits. Offhand, I'm not 
sure what the implementation will do here; returning infinity or a hex 
string with an extra big exponent are both defensible.


Thanks,

-Joe

Re: Java 8 RFR 8020641: Clean up some code style in recent BigInteger contributions

[Joseph Darcy]

Hi Brian,

Acknowledged, sounds good; thanks,

-Joe

On 7/17/2013 4:42 PM, Brian Burkhalter wrote:

Hi Joe,

I've updated the webrev

http://cr.openjdk.java.net/~bpb/8020641/

to include the minor changes pointed out by Tim

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019033.html

Thanks,

Brian

On Jul 16, 2013, at 11:26 PM, Joe Darcy wrote:


This latest version looks good to go; thanks,

Re: Java 8 RFR 6480539: BigDecimal.stripTrailingZeros() has no effect on zero itself ("0.0")

[Joseph Darcy]

On 7/9/2013 12:46 PM, Brian Burkhalter wrote:

On Jul 9, 2013, at 12:17 PM, Joe Darcy wrote:


If the specification change

 [...] If
2596  * {@code compareMagnitude(BigDecimal.ZERO) == 0}, then
2597  * {@code BigDecimal.ZERO} is returned.

is modified to something like "If this BigDecimal is numerically equal to zero, then 
BigDecimal.ZERO is returned." then the change will be ready to go back.

Done:

http://cr.openjdk.java.net/~bpb/6480539/




That version look fine; thanks,

-Joe

Re: RFC 6910473: BigInteger negative bit length, value range, and future prospects

[Joseph Darcy]

Hello,

A quick note on this issue, before the recent work to use better 
algorithms for BigInteger arithmetic operation, working with huge 
numbers was impractical and thus BigInteger.bitLength misbehavior was 
mostly an academic concern. With the better algorithms, exposure to 
these large values is likely to increase so BigInteger.bitLength should 
do something reasonable.


There are at least two implementation limits of note in the current code:

* Total bit length given a single backing array
* Max size of serializable BigInteger given old byte-array based format.

My preference for addressing this issue includes adding an 
implementation note along the lines of "in JDK 8, BigInteger operates on 
values in the range " without requiring that range to be part of the 
specification.


Cheers,

-Joe

On 6/25/2013 6:18 PM, Brian Burkhalter wrote:

This request for comment regards this issue

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6910473
BigInteger.bigLength() may return negative value for large numbers

but more importantly Dmitry's thread

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018345.html
What is the range of BigInteger values?

The issue may be fixed superficially simply by throwing an ArithmeticException 
if the bit length as an int would be negative. A better fix suggested both in 
the issue description and in the aforementioned thread (option 1 of 3), is to 
define BigInteger to support a limited range and to clamp to that range in the 
constructor, throwing an ArithmeticException if the supplied parameter(s) would 
cause the BigInteger to overflow. This check would be relatively inexpensive. 
The suggested constraint range is
[ -pow(2, pow(2,31) - 1), pow(2, pow(2,31) - 1) )
This constraint would guarantee that all BigInteger instances are capable of 
accurately returning their properties such as bit length, bit count, etc.

This naturally would require a minor specification change to BigInteger. The 
question is whether this change would limit any future developments of this and 
related classes. For example, one could envision BigInteger supporting bit 
lengths representable by a long instead of an int. In this case the second 
option would be preferable.

It has been suggested that an alternative to extending the ranges supported by 
BigInteger would be to define a different class altogether to handle the larger 
ranges, keeping BigInteger as a well-specified API for the ranges it supports. 
Other related classes for arbitrary precision binary floating point and 
rational numbers might also be considered.

In summary the specific questions being posed here are:

1) what is the general opinion on clamping the range of BigInteger and the 
various options suggested at the end of

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018345.html ?

2) what are the forward looking thoughts on handling integers outside the 
current BigInteger range?

 From a practical point of view, any changes need to be considered with respect 
to what may be done in the short term (JDK 8) versus the long (JDK 9), so to 
speak.

Thanks,

Brian

Re: JDK-8016285: Add java.lang.reflect.Parameter.isNamePresent()

[Joseph Darcy]

We generally don't delve into low-level presentation details, but the 
change looks fine -- approved to go back.


Cheers,

-Joe

On 7/1/2013 8:04 AM, Eric McCorkle wrote:

Pinging this one again...

On 06/24/13 15:20, Eric McCorkle wrote:

Pinging this RFR.  It still needs a capital R reviewer.
http://cr.openjdk.java.net/~emc/8016285/

On 06/21/13 19:21, Eric McCorkle wrote:

On 06/21/13 16:15, Aleksey Shipilev wrote:

On 06/21/2013 11:57 PM, Eric McCorkle wrote:

The webrev is here:
http://cr.openjdk.java.net/~emc/8016285/

Looks generally good (but not a Reviewer).

A few questions though:
   a) Are we em-bracing the brace-less control flow blocks?

Fixed it.


   b) Should hasRealParameterData be transient?
   c) Should hasRealParameterData be volatile? (now is being implicitly
protected by $parameters volatile write/read, but it is fragile)

Transient yes.  Volatile, I'd say I think not, since It's written before
it's read on all possible paths, and all writes are the same value.
But...  I set it volatile just to be safe from reordering problems.

Webrev's been updated.
http://cr.openjdk.java.net/~emc/8016285/

Re: RFR : 7129185 : (M) Add Collections.{checked|empty|unmodifiable}Navigable{Map|Set}

[Joseph Darcy]

On 6/7/2013 2:51 PM, Martin Buchholz wrote:

 is denigrated in favor of {@code ?


It is by me anyway!

-Joe

Re: Review request for 8016101, javadoc typos for SerialRef and SerialStruct

[Joseph Darcy]

Hi Lance,

Looks fine; approved.

Cheers,

-Joe

On 6/6/2013 1:41 PM, Lance Andersen - Oracle wrote:

Hi Aleksey,

The change is 2 lines, not worth a webrev.  Here it is again in case it was 
truncated in last email (looked OK on my end)

  
new-host-2:serial lanceandersen$ hg diff

diff -r b4742d038100 src/share/classes/javax/sql/rowset/serial/SerialRef.java
--- a/src/share/classes/javax/sql/rowset/serial/SerialRef.java  Tue May 28 
15:22:30 2013 +0200
+++ b/src/share/classes/javax/sql/rowset/serial/SerialRef.java  Thu Jun 06 
16:01:07 2013 -0400
@@ -202,7 +202,7 @@
  }
  
  /**

- * Returns a clone of this {@code SerialRef}. .
+ * Returns a clone of this {@code SerialRef}.
   * The underlying {@code Ref} object will be set to null.
   *
   * @return  a clone of this SerialRef
diff -r b4742d038100 src/share/classes/javax/sql/rowset/serial/SerialStruct.java
--- a/src/share/classes/javax/sql/rowset/serial/SerialStruct.java   Tue May 
28 15:22:30 2013 +0200
+++ b/src/share/classes/javax/sql/rowset/serial/SerialStruct.java   Thu Jun 
06 16:01:07 2013 -0400
@@ -87,6 +87,7 @@
   * object for custom mapping the SQL structured type or any of its
   * attributes that are SQL structured types.
   *
+ * @param in an instance of {@code Struct}
   * @param map a java.util.Map object in which
   *each entry consists of 1) a String object
   *giving the fully qualified name of a UDT and 2) the
On Jun 6, 2013, at 4:34 PM, Aleksey Shipilev wrote:


Hi Lance,

On 06/07/2013 12:09 AM, Lance Andersen - Oracle wrote:

Need a reviewer for this minor javadoc update for bug 8016101

(not a reviewer) The patch appears to be truncated, can you submit it as
patch/webrev somewhere on the web (e.g. cr.openjdk.java.net)?

-Aleksey.




Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: JDK 8 code review request for 8014836: Have GenericDeclaration extend AnnotatedElement

[Joseph Darcy]

Hi Remi,

On 5/22/2013 8:36 AM, Remi Forax wrote:

On 05/21/2013 03:16 AM, Joseph Darcy wrote:

Hi Remi,

On 5/20/2013 2:28 PM, Remi Forax wrote:

On 05/20/2013 11:10 PM, Joe Darcy wrote:

Hello,

Please review the patch below which implements

8014836: Have GenericDeclaration extend AnnotatedElement

All the existing implementations of GenericDeclaration in the JDK 
already implement AnnotatedElement. Some code in java.lang.Class 
needed to be adjusted slightly since AnnotatedElement declares a 
default method and calling an interface's default method in an 
implementing class has to go through the direct interface type.


GenericDeclaration is a public interface, so you will break the code 
of everyone that implements it.

By example, Guava's Invokable also implements GenericDeclaration
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/reflect/Invokable.html 



That is technically true; in effect adding several methods to 
GenericDeclaration would break implementations outside of the JDK 
that do not already implement annotated element.


However, this is (only) a source incompatible change and not a binary 
incompatible change so is possible in-bounds for a platform release 
like JDK 8:


http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/OpenJdkDevelopersGuide.v0.777.html#general_evolution_policy 



The Invokable type you reference is the moral equivalent of the 
"Executable" superclass of Constructor and Method added earlier in 
JDK 8. The Invokable type itself is marked as "@Beta" to warn users 
that it could change or even to away. Therefore, I'm not very 
concerned about "breaking" a type like this, which could easily be 
retrofitted to also implement AnnotatedElement as it already a 
wrapper around a Method or a Constructor.


For me, it's not just a source incompatible change,
let suppose that I use Guava reflection API in my program and the jdk8,
I will be able to get a GenericDeclaration  that will fail at runtime 
if I try to call a method of AnnotatedElement on it.

So this change make the existing Guava library not forward compatible.


Again, that part of Guava is marked as "@Beta" to warn against relying 
on it too much. The JDK generally makes weaker promises about runtime 
reflection modeling as compared to compile-time or non-reflective 
runtime behavior.




With the introduction of default methods, we now have a way to make 
change in interfaces backward compatible,

I don't say that we should use default methods here, I don't know.
But I think it worth to think about that.

I've just seen that AnnotatedElement also includes new new abstract 
methods.


For now, I'm going to push the changes to GenericDeclaration as-is. If 
there are undo compatibility issues, default methods for the 
AnnotatedElement methods could be added later in 8 (but they would be a 
bit disappointing since they could have to either return nothing or 
throw Unsupporte-Operation-Exception.)


Thanks,

-Joe





(Also interesting to see a "Parameter" class in Guava given that we 
have added a Parameter class to java.lang.reflect.)


yes, but there is no getName() :)


More reason to upgrade to JDK 8 :-)





Thanks,

-Joe


cheers,
Rémi







Thanks,

-Joe


Rémi



--- a/src/share/classes/java/lang/Class.javaMon May 20 11:56:46 
2013 -0700
+++ b/src/share/classes/java/lang/Class.javaMon May 20 14:07:15 
2013 -0700

@@ -28,6 +28,7 @@
 import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Array;
 import java.lang.reflect.GenericArrayType;
+import java.lang.reflect.GenericDeclaration;
 import java.lang.reflect.Member;
 import java.lang.reflect.Field;
 import java.lang.reflect.Executable;
@@ -115,9 +116,9 @@
  * @since   JDK1.0
  */
 public final class Class implements java.io.Serializable,
- java.lang.reflect.GenericDeclaration,
-  java.lang.reflect.Type,
- java.lang.reflect.AnnotatedElement {
+  GenericDeclaration,
+  Type,
+  AnnotatedElement {
 private static final int ANNOTATION= 0x2000;
 private static final int ENUM  = 0x4000;
 private static final int SYNTHETIC = 0x1000;
@@ -3182,7 +3183,7 @@
  */
 @Override
 public boolean isAnnotationPresent(Class 
annotationClass) {
-return 
AnnotatedElement.super.isAnnotationPresent(annotationClass);
+return 
GenericDeclaration.super.isAnnotationPresent(annotationClass);

 }

 /**
diff -r 6a9148865139 
src/share/classes/java/lang/reflect/GenericDeclaration.java
--- a/src/share/classes/java/lang/reflect/GenericDeclaration.java 
Mon May 20 11:56:46 2013 -0700
+++ b/src/share/classes/java/lang/reflect/GenericDeclaration.java 
Mon May 20 14:07:15 2013 -0700

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2011, Oracle and/or its affiliates. All 
rights reserved.
+ *

Re: RFR : 8007398 : (S) Performance improvements for Int/Long toString() at Radix 2, 8, 16

[Joseph Darcy]

Hello Mike,

The TestNG data provider framework is unfamiliar to me, so my first 
reaction is that it is overkill for this problem as opposed to a one-off 
approach, but that may be driven my lack of experience with it.


However, in the test code

 153 private static final long[] SOME_PRIMES = {
 154 3L, 5L, 7L, 11L, 13L, 17L, 19L, 23L, 29L, 31L, 37L, 41L, 
43L, 47L, 53L,

 155 59L, 61L, 71L, 73L, 79L, Long.MIN_VALUE };

the name of the array is incorrect since Long.MIN_VALUE = -2^63 which is 
composite. Perhaps you were thinking of Integer.MAX_VALUE which is a 
Mersenne prime? (2^61 -1 is a Mersenne prime; 2^63 - 1 is not.)


-Joe

On 5/21/2013 3:45 PM, Mike Duigou wrote:

Ping!

I need a final review on this issue.

Thanks,

Mike

On May 16 2013, at 14:02 , Mike Duigou wrote:


On May 15 2013, at 19:09 , Joseph Darcy wrote:


Hi Mike,

Looks fine. Are you satisfied with the test coverage provided by the existing 
regression tests?

I hadn't actually looked but it turns out the answer was "No, certainly not".

I built a set of tests across all of the primitive integral types that compares 
the results of various toString operations against BigInteger.toString(radix). 
This is not ideal because BigInteger.toString(radix) is implemented via 
Long.toString(l, radix). If there's a different exemplar provider which 
generates results entirely independently of the code paths to be tested the 
test can be fairly easily switched to use that.

If anyone has suggestions for interesting test cases in numberProvider() I 
would also be interested in hearing about those.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/2/webrev/

Mike


-Joe

On 5/15/2013 6:17 PM, Mike Duigou wrote:

Hello all;

This issue was originally part of JDK-8006627 (improve performance of UUID 
parsing/formatting) but was split out because it could be split out. I've been 
working incrementally on pieces of 8006627 as my time permits.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/1/webrev/

I've done microbenchmarking of using digits table vs calculating digits, previously 
mentioned in 
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/016526.html>, 
and found that using the digits table was still faster.

I've also benchmarked removing the offset param from formatUnsignedLong() and 
simplifying the loop termination logic but neither of these turned out to have 
little benefit.

Since the last rev I have made a separate implementation 
Integer.formatUnsignedInteger for use by Integer rather than sharing the Long 
implementation because it's about 30% faster. I suspect the benefits would be 
even greater on a 32-bit VM or 32-bit native platform.

We'll get back to 8006627 soon enough. (I have been tempted to split it again 
into parsing and formatting).

Thanks,

Mike

Re: RFR : 8014819 : set max size for jtreg test vms

[Joseph Darcy]

Looks fine.

-Joe

On 5/21/2013 3:42 PM, Mike Duigou wrote:

Hello all;

A lot more people have been playing with using concurrency lately with JTReg and most 
have found that tests will frequently fail or error out because of OOM errors. The 
problem is that the jdk/test/Makefile currently doesn't specify a size for the vm 
instances used for running tests. This results in "VM Ergonomics" sizing being 
used and the test VMs which get created are generally too large to run very many of them.

This patch sets a specific size to the vm instances created to run tests, 
512MB. This value has been successfully used to run tests by our internal SQE 
team and by Alan Bateman for quite some time. It also seems to work for me. 
Individual tests which require more memory can specify their requirements using 
explicit -Xmx options in @run tags.

http://cr.openjdk.java.net/~mduigou/JDK-8014819/0/webrev/

Thanks,

Mike

Re: JDK 8 code review request for 8014836: Have GenericDeclaration extend AnnotatedElement

[Joseph Darcy]

Hi Remi,

On 5/20/2013 2:28 PM, Remi Forax wrote:

On 05/20/2013 11:10 PM, Joe Darcy wrote:

Hello,

Please review the patch below which implements

8014836: Have GenericDeclaration extend AnnotatedElement

All the existing implementations of GenericDeclaration in the JDK 
already implement AnnotatedElement. Some code in java.lang.Class 
needed to be adjusted slightly since AnnotatedElement declares a 
default method and calling an interface's default method in an 
implementing class has to go through the direct interface type.


GenericDeclaration is a public interface, so you will break the code 
of everyone that implements it.

By example, Guava's Invokable also implements GenericDeclaration
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/reflect/Invokable.html 



That is technically true; in effect adding several methods to 
GenericDeclaration would break implementations outside of the JDK that 
do not already implement annotated element.


However, this is (only) a source incompatible change and not a binary 
incompatible change so is possible in-bounds for a platform release like 
JDK 8:


http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/OpenJdkDevelopersGuide.v0.777.html#general_evolution_policy

The Invokable type you reference is the moral equivalent of the 
"Executable" superclass of Constructor and Method added earlier in JDK 
8. The Invokable type itself is marked as "@Beta" to warn users that it 
could change or even to away. Therefore, I'm not very concerned about 
"breaking" a type like this, which could easily be retrofitted to also 
implement AnnotatedElement as it already a wrapper around a Method or a 
Constructor.


(Also interesting to see a "Parameter" class in Guava given that we have 
added a Parameter class to java.lang.reflect.)


Thanks,

-Joe





Thanks,

-Joe


Rémi



--- a/src/share/classes/java/lang/Class.javaMon May 20 11:56:46 
2013 -0700
+++ b/src/share/classes/java/lang/Class.javaMon May 20 14:07:15 
2013 -0700

@@ -28,6 +28,7 @@
 import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Array;
 import java.lang.reflect.GenericArrayType;
+import java.lang.reflect.GenericDeclaration;
 import java.lang.reflect.Member;
 import java.lang.reflect.Field;
 import java.lang.reflect.Executable;
@@ -115,9 +116,9 @@
  * @since   JDK1.0
  */
 public final class Class implements java.io.Serializable,
- java.lang.reflect.GenericDeclaration,
-  java.lang.reflect.Type,
- java.lang.reflect.AnnotatedElement {
+  GenericDeclaration,
+  Type,
+  AnnotatedElement {
 private static final int ANNOTATION= 0x2000;
 private static final int ENUM  = 0x4000;
 private static final int SYNTHETIC = 0x1000;
@@ -3182,7 +3183,7 @@
  */
 @Override
 public boolean isAnnotationPresent(Class 
annotationClass) {
-return 
AnnotatedElement.super.isAnnotationPresent(annotationClass);
+return 
GenericDeclaration.super.isAnnotationPresent(annotationClass);

 }

 /**
diff -r 6a9148865139 
src/share/classes/java/lang/reflect/GenericDeclaration.java
--- a/src/share/classes/java/lang/reflect/GenericDeclaration.java Mon 
May 20 11:56:46 2013 -0700
+++ b/src/share/classes/java/lang/reflect/GenericDeclaration.java Mon 
May 20 14:07:15 2013 -0700

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2011, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,7 +30,7 @@
  *
  * @since 1.5
  */
-public interface GenericDeclaration {
+public interface GenericDeclaration extends AnnotatedElement {
 /**
  * Returns an array of {@code TypeVariable} objects that
  * represent the type variables declared by the generic

Re: RFR : 8007398 : (S) Performance improvements for Int/Long toString() at Radix 2, 8, 16

[Joseph Darcy]

Hi Mike,

Looks fine. Are you satisfied with the test coverage provided by the 
existing regression tests?


-Joe

On 5/15/2013 6:17 PM, Mike Duigou wrote:

Hello all;

This issue was originally part of JDK-8006627 (improve performance of UUID 
parsing/formatting) but was split out because it could be split out. I've been 
working incrementally on pieces of 8006627 as my time permits.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/1/webrev/

I've done microbenchmarking of using digits table vs calculating digits, previously 
mentioned in 
, 
and found that using the digits table was still faster.

I've also benchmarked removing the offset param from formatUnsignedLong() and 
simplifying the loop termination logic but neither of these turned out to have 
little benefit.

Since the last rev I have made a separate implementation 
Integer.formatUnsignedInteger for use by Integer rather than sharing the Long 
implementation because it's about 30% faster. I suspect the benefits would be 
even greater on a 32-bit VM or 32-bit native platform.

We'll get back to 8006627 soon enough. (I have been tempted to split it again 
into parsing and formatting).

Thanks,

Mike

Re: Review Request for JDK-8012937: Correct errors in javadoc comments

[Joseph Darcy]

Acknowledged; thanks for checking,

-Joe

On 4/23/2013 7:46 AM, Eric McCorkle wrote:

I believe so.  Alex Buckley recommended the exact wording.

On 04/22/13 22:09, Joseph Darcy wrote:

Hello,

  240  * Returns the number of formal parameters (whether explicitly
  241  * declared or implicitly declared or neither) for the executable

Are there parameters that are neither explicitly nor implicitly declared?

I still think the follow comment is better deleted given the source that
follows it:

  157 // If a parameter has no name, return argX, where x is the
  158 // index.
  159 //

-Joe

On 4/22/2013 11:46 AM, Eric McCorkle wrote:

I have posted a newer version with some more edits.  Please review and
suggest any further changes.

http://cr.openjdk.java.net/~emc/8012937/webrev.01/

On 04/22/13 12:10, Eric McCorkle wrote:

Hello,

Please review this simple change, which corrects some errors in the
javadoc comments for method parameter reflection.

Note that this changeset does not include any code changes.

The webrev is here:
http://cr.openjdk.java.net/~emc/8012937/webrev.00/


Also, if you have any additional issues with the javadoc comments,
please reply to this request with a description of the problem.

Thanks,
Eric

Re: Review Request for JDK-8012937: Correct errors in javadoc comments

[Joseph Darcy]

Hello,

 240  * Returns the number of formal parameters (whether explicitly
 241  * declared or implicitly declared or neither) for the executable

Are there parameters that are neither explicitly nor implicitly declared?

I still think the follow comment is better deleted given the source that 
follows it:


 157 // If a parameter has no name, return argX, where x is the
 158 // index.
 159 //

-Joe

On 4/22/2013 11:46 AM, Eric McCorkle wrote:

I have posted a newer version with some more edits.  Please review and
suggest any further changes.

http://cr.openjdk.java.net/~emc/8012937/webrev.01/

On 04/22/13 12:10, Eric McCorkle wrote:

Hello,

Please review this simple change, which corrects some errors in the
javadoc comments for method parameter reflection.

Note that this changeset does not include any code changes.

The webrev is here:
http://cr.openjdk.java.net/~emc/8012937/webrev.00/


Also, if you have any additional issues with the javadoc comments,
please reply to this request with a description of the problem.

Thanks,
Eric

Re: REASSERT Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed

[Joseph Darcy]

Hello,

Just reasserting the request for a review of the latest version of this 
patch:


http://cr.openjdk.java.net/~darcy/8012044.2

I believe this version does an appropriate job of propagating exception 
information when there is misuse of the methods on Throwable.


Thanks,

-Joe

On 4/17/2013 10:32 AM, Joe Darcy wrote:

On 04/14/2013 07:36 PM, Joe Darcy wrote:

On 04/12/2013 07:29 PM, Jason Mehrens wrote:

Joe,
You'll have guard ise.addSuppressed against null.  Looks good 
otherwise.


private static void initCauseNull() {
  Throwable t1 = new Throwable();
  t1.initCause(null);
  try {
t1.initCause(null);
  } catch(IllegalStateException expect) {
  }
}


Right you are; check added and test updated in:

http://cr.openjdk.java.net/~darcy/8012044.2/

Full patch to Throwable:


[snip]

One more iteration; I've changed the initCause logic to suppress both 
exceptions rather than using one as the cause:


 http://cr.openjdk.java.net/~darcy/8012044.2

Patch to throwable:

--- old/src/share/classes/java/lang/Throwable.java2013-04-14 
19:33:23.0 -0700
+++ new/src/share/classes/java/lang/Throwable.java2013-04-14 
19:33:23.0 -0700

@@ -452,10 +452,15 @@
  * @since  1.4
  */
 public synchronized Throwable initCause(Throwable cause) {
-if (this.cause != this)
-throw new IllegalStateException("Can't overwrite cause");
+if (this.cause != this) {
+IllegalStateException ise =
+new IllegalStateException("Can't overwrite cause", 
this);

+if (cause != null)
+ise.addSuppressed(cause);
+throw ise;
+}
 if (cause == this)
-throw new IllegalArgumentException("Self-causation not 
permitted");
+throw new IllegalArgumentException("Self-causation not 
permitted", this);

 this.cause = cause;
 return this;
 }
@@ -1039,7 +1044,7 @@
  */
 public final synchronized void addSuppressed(Throwable exception) {
 if (exception == this)
-throw new 
IllegalArgumentException(SELF_SUPPRESSION_MESSAGE);
+throw new 
IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, exception);


 if (exception == null)
 throw new NullPointerException(NULL_CAUSE_MESSAGE);

The suppression mechanism is typically, but not exclusively, used by 
the try-with-resources statement.


Thanks,

-Joe

Re: JDK 8 code review request for 8011800: Add java.util.Objects.requireNonNull(T, Supplier)

[Joseph Darcy]

On 4/10/2013 5:02 AM, Remi Forax wrote:

On 04/09/2013 11:12 PM, Joe Darcy wrote:

Hello,

Please review my changes for

8011800: Add java.util.Objects.requireNonNull(T, Supplier)
http://cr.openjdk.java.net/~darcy/8011800.0/

which add a new method to java.util.Objects to take a 
Supplier rather than a String.


Patch inline below.

Thanks,

-Joe


It's premature in my opinion to introduce this kind of method in the API.
The cost of creating a lambda the first time is actually even worst as 
the one
of loading an inner class. Objects.requireNonNull should be a quick 
check,

not something that involve to load a new class.

It's true that the JIT will optimize the lambda creation but there are 
lot of codes
that are never JITed, typically less than 20% of the code of a program 
is JITed.


Adding this API will slow down the startup time of a program,
Java is known to be slow at startup, in fact, the VM is not that slow, 
the JDK and
the application are slow to startup. It's not a good idea to provide a 
way

to make the startup of an application slower that it needs to be.


Acting as legal counsel for Objects.requireNonNull(T, Supplier), 
I submit a plea of "not guilty" to the charge of slowing down Java 
startup. First, the method isn't in the JDK yet. Second, even if it were 
in the JDK, it would first have to be used in a startup-critical way. 
Third, the method contains a disclaimer warning against misuse.


-Joe

Re: @Supported design issues

[Joseph Darcy]

On 3/14/2013 10:14 AM, Joe Darcy wrote:
FYI, I've submitted a JEP reviewed by Alan Bateman on "Capturing 
support and stability information about JDK classes in source and 
class files."




Following up in the same thread, the JEP for this work is now available 
for your reading pleasure at:


JEP 179: Document JDK API Support and Stability
http://openjdk.java.net/jeps/179

-Joe

Re: java.lang.reflect.AnnotatedElement changes

[Joseph Darcy]

On 2/15/2013 3:49 PM, Martin Buchholz wrote:

On Fri, Feb 15, 2013 at 6:10 AM, David Holmes wrote:


On 15/02/2013 11:53 PM, Kasper Nielsen wrote:


Hi,

I know AnnotatedElement is not a common class to implement outside of the
JDK. But the interface introduces some new methods that will break
existing
code.


I must admit I am surprised as I thought that any new methods on
interfaces would have to have default implementations to avoid this kind of
breakage. Seems to me that defaults should be possible for those new
methods. Perhaps this is still a work in progress?


OpenJDK could invest more in static analysis tools.  Here it would be good
to use a tool to audit all interfaces to check that new methods without
defaults have not been added.


Hi Martin,

FWIW, with my ccc hat on, I've previously (internally) asked the JCK 
team to run such reports. IIRC, the methods on AnnotatedElement where 
the only potentially problematic ones made so far. (We'll be added 
methods to interfaces over in javax.lang.model.*, but we can't use 
default methods over than and we explicitly warn implementers and user 
of the API of the potential for new methods.)


Thanks,

-Joe

Re: RFR - 6480539: BigDecimal.stripTrailingZeros() should specify no-op on zero BigDecimals

[Joseph Darcy]

On 2/4/2013 1:36 PM, Stephen Colebourne wrote:

On 4 February 2013 19:31, Joe Darcy  wrote:

The stripTrailingZeros method has acted in this surprising way since the
IBM-led JSR 13 was integrated into the platform back in JDK 5, which shipped
in 2004.

This situation is analogous to when the specification and behavior disagree.
Our general policy to resolve such cases when evolving the JDK is:

"..., there are times in evolving the JDK when differences are found between
the specified behavior and the actual behavior (for example 4707389,
6365176). The two basic approaches to fixing these bugs are to change the
implementation to match the specified behavior or to change the
specification (in a platform release) to match the implementation's (perhaps
long-standing) behavior; often the latter option is chosen since it has a
lower de facto impact on behavioral compatibility."
http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/OpenJdkDevelopersGuide.v0.777.html

If this issue were being addressed before JDK 5 shipped or even during JDK
6, I would support changing the behavior of stripTrailingZeros.  However,
for addressing this in JDK 8, I think it is more appropriate to keep the
behavior as-is and document this special case.

I don't see what JDK 5 vs 6 vs 8 really has to do with it.


The longer a particular behavior has been in a shipped JDK release, the 
more likely it is various people have built code that depend on that 
behavior.


-Joe

Re: Request for Review JDK-8006503:JVM_PrintStackTrace is not used in JDK

[Joseph Darcy]

Looks fine as far as I can see.

Thanks,

-Joe

On 1/24/2013 3:30 PM, Eric McCorkle wrote:

As far as Coleen and I are aware, this is internal-only.  If anyone
knows otherwise, please comment before the end of tomorrow.

Other than that, do you see any problems, Joe?

On 01/24/13 15:41, Joe Darcy wrote:

If this function was only usable with the JDK implementation (an
internal JDK contract), then a ccc request is not needed.

HTH,

-Joe

On 1/24/2013 12:26 PM, Eric McCorkle wrote:

I think the question was more of "does one need to be filed?"

On 01/24/13 15:15, Joe Darcy wrote:

There is as of yet no ccc request for 8006503.

-Joe

On 1/24/2013 12:11 PM, Eric McCorkle wrote:

Joe, can you comment on this?

On 01/24/13 14:59, Coleen Phillimore wrote:

What was the disposition of the CCC request?   Was it decided not
to do
one because this function hasn't been used since before 1.4?

Code looks good to me.  I removed the one in hotspot last week.
Thanks,
Coleen

On 01/24/2013 10:59 AM, Eric McCorkle wrote:

Hello,

Please review this trivial patch which removes JVM_PrintStackTrace
from
jvm.h.  It is no longer being used.

The webrev is here:
http://cr.openjdk.java.net/~emc/8006503/webrev.00/

The bug is here:
http://bugs.sun.com/view_bug.do?bug_id=8006503

Thanks,
Eric

Re: Review Request: 8004201: add reducers to primitive type wrappers

[Joseph Darcy]

Akhil,

In javadoc like this

298  * as {@code BinaryOperator}.

you don't have to use "<" and the like inside {@code}; please change to

298  * as {@code BinaryOperator}.

As a general comment, if the operations for primitive type Foo are put 
into java.lang.Foo, then the type of the operation needs to be 
explicitly represented in the expression calling the method (unless 
static imports are used, etc.).  Therefore, I suggest putting these sort 
of static methods all into one class to allow overloading to do its 
thing (java.lang.Operators?).  Also, for methods like sum, I think it is 
worth considering returning a larger type than the operands to avoid 
problems from integer or floating-point overflow. For example, sum on 
byte and short would return int, sun on int would return long, etc.


As an aside, to get a numerically robust result, the summation logic 
over a set of doubles needs to be more than just a set of raw double 
additions, but that can be tackled later.


Cheers,

-Joe


On 12/5/2012 1:27 PM, Akhil Arora wrote:

Updated - http://cr.openjdk.java.net/~akhil/8004201.1/webrev/

- delegate to Math.min/max for int/long/float/double
- rename Boolean.and/or/xor to logicalAnd/logicalOr/logicalXor
- removed Character variants of min/max/sum

On 12/02/2012 05:50 PM, David Holmes wrote:

Hi Akhil,

Is it really necessary/desirable to flag all of these as " Suitable for
conversion as a method reference to functional interfaces such as ..." ?

Not necessary, but it does provide a hint as to their intended use to a
casual browser of these docs.


This style:

+ * @param   a   a boolean argument.
+ * @param   b   another boolean argument.

is at odds with the style used elsewhere for new Functional APIs, and
with the style of other methods in these classes. Can we just use "first
operand" and "second operand" for all of these?

It is consistent with Math.min/max, which use the a/b style. Since these
methods are not in one of the functional package, is'nt it better to
stick to the local style?


Character.sum does not make sense to me. Who adds together characters?
I'm not even sure min and max are worth supporting for Character.

Good point - removed these methods for Character.


I disagree with other suggestions to use the Math functions for
float/double. I think all these methods should use the underlying
primitive operator regardless of type.

Are you disagreeing only for float/double or for int/long also? Can you
provide more information as to why you disagree?

Thanks


Thanks,
David
-

On 1/12/2012 4:44 AM, Akhil Arora wrote:

Hi

Requesting review for some basic functionality related to lambdas -

Add min, max, sum methods to the primitive wrapper classes - Byte,
Short, Integer, Long, Float, Double and Character so as to be able to
use them as reducers in lambda expressions. Add and, or, xor methods to
Boolean.

http://cr.openjdk.java.net/~akhil/8004201.0/webrev/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004201

Thanks

Re: Reviewer needed: 6282196 There should be Math.mod(number, modulo) methods

[Joseph Darcy]

Hi Roger,

The changes look fine.  However, I suggest adding an explanatory note 
along the lines of "Normal integer division operates under the round to 
zero rounding mode (truncation).  This operation instead acts under the 
round to negative infinity (floor) rounding mode. The floor rounding 
mode gives different results than truncation when the exact result is 
negative."


Thanks,

-Joe

On 10/10/2012 7:22 AM, Roger Riggs wrote:

A reviewer is needed for:

6282196 There should be Math.mod(number, modulo) methods

The webrev is: http://cr.openjdk.java.net/~rriggs/6282196.4/

Thanks, Roger

Re: [PATCH] Sunbug 7131192: Optimize BigInteger.doubleValue(), floatValue()

[Joseph Darcy]

Hello,

Thanks for the patch Louis.

On 7/12/2012 3:21 AM, Andrew Haley wrote:

On 07/12/2012 10:32 AM, Louis Wasserman wrote:

It was attached to the previous message?  I don't know if this list works
with attachments.  Alternately, the patch was attached here:
https://bugs.openjdk.java.net/show_bug.cgi?id=100222

I'm not sure what you mean by double-rounding bugs, though.  It's
not difficult to actually implement the HALF_EVEN rounding behavior
with bit twiddling.

Sure, as long as you've thought about it and done it carefully.  The
bit twiddling is easy to do, and easy to get wrong.

> From the supplied patch it looks like you've done a good job, but
there was no way to tell without it.  I presume the listserv dropped
it on the floor.

Andrew.


I've taken a quick look at the patch.  The concept for the change is 
good; the current path of converting to float/double through a string is 
a simple but very roundabout way to accomplish this task.


Unfortunately, I'm saturated with the JDK bug migration [1] and will 
continue to be saturated for at least several more weeks so I won't be 
able to take a more detailed look at the patch for a while.  I suspect 
some more directly test cases will be needed to test tricky rounding 
situations.


Thanks,

-Joe

[1] https://blogs.oracle.com/darcy/entry/moving_monarchs_dragons

Re: Code Review Request: 7170169: (props) System.getProperty("os.name") should return "Windows 8" when run on Windows 8

[Joseph Darcy]

Hi Kurchi,

Looks fine,

-Joe

On 5/22/2012 6:32 PM, Kurchi Hazra wrote:

Hi,

  This is a minor change to java_props_md.c to return "Windows 8" as 
the "os.name" for Windows version 6.2.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7170169
Webrev: http://cr.openjdk.java.net/~khazra/7170169/webrev.00/

Thanks,
Kurchi

Re: Remove the assert in Integer.valueOf()

[Joseph Darcy]

Hi Remi,

On 4/27/2012 3:38 PM, Rémi Forax wrote:

On 04/27/2012 05:29 AM, Joe Darcy wrote:

On 4/24/2012 8:01 AM, Alan Bateman wrote:

On 24/04/2012 14:56, Rémi Forax wrote:


Here, I don't really ask for tweaking something but more to remove 
an assert

which do something which is unrelated to the current algorithm.
In my opinion, it's a debug assert used during the development
that slip into the production code. The fact that the range [-128, 
127] should be
covered by the cache is mandated by the JLS, but if you really want 
an assert

you should move it when high and low are initialized (they are final)
and not where they are used.

I also agree that the inlining heuristic should be changed and
I'm sure that everybody that have looked to the inlining heuristic 
code of Hotspot will agree with that

but this is somehow unrelated to the problem.
This is how I've found this assert,
it doesn't change the fact that this assert should not be there.
I added that assert as part of the work of 6807702. You can't use 
asserts in code that is executed before system initialization is 
complete and I think this is why we put the assert is valueOf rather 
than in the initializer. I don't have any objection to removing it, 
assuming Joe is okay with it too.


-Alan

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4c3f752993a5


Hi Alan,

I don't object to the assert being removed if asserts or 
assert-equivalent checks are added in the IntegerCache class.


I have moved the assert into the static block of IntegerCache.
For Alan, because IntegerCache is loaded when Integer.valueOf() is 
called the first time
the assert code is checked around the same time so after the system 
init but only once.


webrev is here:
  http://cr.openjdk.java.net/~forax/integer_valueof/
and I need a bug for it :)


Here is your bug:

7165102 Only run assertion on Integer autoboxing cache size once

However, I'd prefer the assert appear right after the

 777 high = h;

assignment.

Cheers,

-Joe

Re: Reviewer needed: 6282196 There should be Math.mod(number, modulo) methods

[Joseph Darcy]

Hi Roger,

In a case like this where the Math and StrictMath versiosn of a method 
have exactly the same behavior defined, I strongly suggest having the 
Math version of the test invoke tests for both Math and StrictMath 
siblings of a method.  In other words, have a single


test/java/lang/Math/DivModTests.java

file testing both Math and StrictMath methods; this should ease 
maintenance of the tests going forward.  Otherwise the code looks fine.  
However, I suggest adding a short paragraph to discuss the div & mod 
methods to explain how div and mod are related (mod is paired to div so 
that (a div b) * b + (a mod b) = a) and floor div and floor mod how they 
differ from the built-in / and % operators.


Thanks,

-Joe

On 4/16/2012 2:08 PM, Roger Riggs wrote:

Hi,

I've corrected a number of issues raised with javadoc in java.lang.Math
and java.lang.StrictMath.  The updated webrev is:
 http://cr.openjdk.java.net/~rriggs/6282196.2/

Please review.

Thanks, Roger


On 03/05/2012 08:38 AM, Roger Riggs wrote:

CR 6282196 There should be Math.mod(number, modulo) methods
  http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6282196

Requests that floor and modulus methods be provided for primitive types.
Floor division is pretty straight-forward, rounding toward minus 
infinity.
For modulus of int and long, the sign and range follow  the exiting 
floor method
in java.util.Math and satisfy the relation that mod(x, y) = (x - 
floorDiv(x, y) * y).

Re: Please review 7156976: improve java tools testing

[Joseph Darcy]

Looks fine.

Cheers,

-Joe

On 3/28/2012 10:39 AM, Kumar Srinivasan wrote:

Hi,

We are adding new test to test the tool functionality of the launcher:

1. verify the options intended for a tool does gets to it intact, 
Steve Sides
from SQE has contributed this test, which applies various test 
vectors to

a simulated javac launcher.

2. enhanced the version checking to ensure a tool can be launched

3. The version checker now has inverted logic to select the tool 
selection,
 it will execute all possible tools in the bin directory. Thus any 
new tools
 added will be tested. Therefore, if a new tool is not compliant, 
the onus is on

 the tool developer to:
  a: make the tool compliant.
  b: if a is impossible,  then add it to the excluded list.

http://cr.openjdk.java.net/~ksrini/7156976/webrev/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7156976

Thanks
Kumar & Steve

Re: RFR: 7142617 De-optimize fdlibm compilation [macosx]

[Joseph Darcy]

Looks fine,

-Joe

On 2/3/2012 1:23 PM, Michael McMahon wrote:

Can I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/7142617/webrev.1/

fdlibm needs to be compiled with optimization disabled, as on Linux.

Thanks
Michael.

Re: Review request : 7141141 Add 3 new test scenarios for testing Main-Class attribute in jar manifest file

[Joseph Darcy]

Hello,

On 2/1/2012 1:37 PM, Kumar Srinivasan wrote:

Hi,

Here are some improvements to the launcher tests, contributed by Sonali
Goel of the SQE team, please review:

http://cr.openjdk.java.net/~ksrini/7141141/webrev.0/

The CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7141141

Thanks
Kumar



A few stylistic comments:

29  * @compile -XDignore.symbol.file MainClassAttributeTest.java

It might suffice to use @build instead of @compile.

  43  * only English is seleced and will pass vacuosly for other locales.
  44  *
  45  */

Extra line.

  98 public static void main(String[] args) throws Throwable {

Is it necessary to throw Throwable here?

Otherwise looks fine.

-Joe

Re: Using unsigned library work in the JDK

[Joseph Darcy]

Hello,

On 1/26/2012 9:13 AM, Ulf Zibis wrote:

Am 26.01.2012 17:11, schrieb Roger Riggs:



[snip]




Its unfortunate that between the method name and need to qualify
with the class (or use static imports) that the code is longer and 
not always clearer.

+1
- static import could become ambiguous using Byte.toUnsignedInt() + 
Short.toUnsignedInt() at same time.

+ The shortest I can think:
Short.unsigned(byte)
Integer.unsigned(short)
Long.unsigned(int)
BigInteger.unsigned(long)
+ Nothing would be ambiguous, using static import.


That style of design was considered for

4504839 Java libraries should provide support for unsigned integer 
arithmetic


However, at least for now, we choose the other approach.

-Joe

Re: Code Review Request Bug #7129185:(coll) Please add Collections.emptyNavigableSet()

[Joseph Darcy]

Hello Darryl,

On 1/23/2012 3:19 PM, Darryl Mocek wrote:
Re-sending this with the synopsis in the subject line (and the correct 
bug #).


Hello core-libs.  Please review this patch to fix Bug #7129185.  This 
fix addresses comments made by Jason Mehrens to the commit of the fix 
for bug #4533691, including adding a Collections.emptyNavigableSet 
method.  Tests are included.


Webrev, can be found here: 
http://cr.openjdk.java.net/~dmocek/7129185/webrev.00





I recommend using some different javadoc idioms.  For example, I'd replace

3209  * 
3210  * NavigableSet ns = 
Collections.emptyNavigableSet();

3211  * 


with


{@code NavigableSet ns = Collections.emptyNavigableSet();}


and replace Foo with {@code Foo}.

-Joe

Re: JDK 8 code review request for initial unsigned integer arithmetic library support

[Joseph Darcy]

On 1/19/2012 8:05 AM, Ulf Zibis wrote:

Am 19.01.2012 07:43, schrieb Eamonn McManus:

Ulf Zibis writes:
> What about:
>  private static final BigInteger BEYOND_UNSIGNED_LONG = 
BigInteger.valueOf(1).shiftLeft(64);

>  private static BigInteger toUnsignedBigInteger(long i) {
>  BigInteger result = BigInteger.valueOf(i);
>  if (i < 0L)
>  result = result.add(BEYOND_UNSIGNED_LONG);
>  return result;
>  }

That's a nice idea! But the problem is that it would mean that 
BigInteger.class would be loaded as soon as Long.class is, which I 
think is undesirable.

Thanks for the critic. I didn't see that.
The problem could be easily avoided if method 
toUnsignedBigInteger(long i) would be moved to class BigInteger as 
unsignedValueOf(long i), as I additionally noted in my last post.



However it does make me think that we could change...to this:

if (i >= 0L) {
return BigInteger.valueOf(i);
} else {
return BigInteger.valueOf(i & Long.MAX_VALUE).setBit(63);
}

Another nice idea!
But again, moving the entire method to BigInteger would additionally 
avoid to clown around with the available BigInteger's public APIs. 
Having the method at BigInteger would allow elegant direct access to 
the private value fields.





If the operation in question starts becoming a bottleneck, these 
alternate implementations can be explored.  In the meantime, I plan to 
stick with the straightforward code and not setup the infrastructure 
needed to get at BigInteger internals.


Thanks,

-Joe

Re: JDK 8 code review request for initial unsigned integer arithmetic library support

[Joseph Darcy]

On 1/18/2012 7:52 PM, Ulf Zibis wrote:

Am 18.01.2012 03:54, schrieb Joe Darcy:

I've posted a revised webrev at

http://cr.openjdk.java.net/~darcy/4504839.2


Instead
'\u0030'
you can use
{@code '\u005Cu0030'}


That is a fine cleanup, but I'll do a bulk conversion of all the 
instances of that pattern later on under another bug.




Byte:
=
 459 public static int toUnsignedInt(byte x) {
 460 return ((int) x) & 0xff;
 461 }
This should be good enough (similar at Short, Integer):
 459 public static int toUnsignedInt(byte x) {
 460 return x & 0xff;
 461 }
(This notation if regularly used in sun.nio.cs coders.)


I think the current code more explicitly indicates the intention of the 
method's operation to more casual readers less familiar with the details 
of primitive widening conversions, etc.





missing:
public static short toUnsignedShort(byte x)


That omission was intentional.  The language and VM only really support 
arithmetic on int and long values, not short or byte, so I only 
providing methods to widen to int and long.




superfluous:
public static long toUnsignedInt(byte x)
public static long toUnsignedLong(byte x) (similar at Short)
one can use:
int i = toUnsignedShort(x)
long l = toUnsignedShort(x) (similar at Short)

Integer:

 623  * The value represented by the string is larger than the
 624  * largest unsigned {@code int}, 232-1.
If you format {@code int}, then you speak about the java type int, 
which is always signed, never unsigned.

IMO you should better write 'unsigned 32-bit integer".
(similar at Long)


Due to similar feedback, I've made various clarifications to the 
javadoc, but the basic situation is that the "fooUnsigned" methods 
interpret the bits as unsigned values, as already done in toHexString 
and related methods.


Thanks,

-Joe

Re: Request for Review: 7094995: test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java causes continuous GC in agentvm mode

[Joseph Darcy]

On 11/24/2011 3:15 AM, Neil Richards wrote:

On Thu, 2011-11-24 at 11:22 +1000, David Holmes wrote:

Hi Joe,

On 24/11/2011 2:33 AM, Joe Darcy wrote:

On 11/22/2011 9:57 PM, David Holmes wrote:

On 22/11/2011 9:51 PM, Neil Richards wrote:

I've also converted the testcase's use of ZipFile, ZipOutputStream&
FileOutputStream to use ARM (for greater clarity).

I think proper use of ARM requires that this:

66 try (ZipOutputStream zos =
67 new ZipOutputStream(new FileOutputStream(tempZipFile))) {

be written as:

try (FileOutputStream fos = new FileOutputStream(tempZipFile);
ZipOutputStream zos = new ZipOutputStream(fos)) {


The more conservative approach is to define one resource variable per
logical resource even if the one resource is "wrapped" by the second
one, as in the second example. This does close the small window of
vulnerability between when the first constructor call ends and the
second one completes. However, if that pattern is used, it is important
for the close method of the inner resource to be idempotent, as required
by the java.io.Closeable type but *not* required by
java.lang.AutoCloseable.

Sorry but I couldn't quite tell what you were recommending there. Is the
original code or my change the preferred approach? As I understood it
the original code would not close the FileOutputStream if the
ZipOutputStream constructor threw an exception.

Thanks,
David

In this instance, I think separating the allocations out into separate
statements in the ARM's try is fine, because both FileOutputStream and
ZipOutputStream are Closeable, and therefore have idempotent [1] close()
methods. (They're obviously also both AutoCloseable, to be used by ARM
in the first place).

-

Consider, however, if FileOutputStream were not Closeable, and therefore
didn't guarantee the idempotency of its close().

(It might then do something like throw an Exception if close() is called
for a second time.)

Then the decision to have it auto-closed by ARM would hinge upon whether
the call to ZipOutputStream's close() causes a close() call to be made
to the (File)OutputStream it holds.

If it does, one would not want to use ARM to (also) call the
(non-Closeable) FileOutputStream's close(), as it would run the risk of
seeing non-idempotent behaviour (eg. an Exception thrown).

-

However, coming back to reality, both objects _are_ Closeable, and so
_do_ have idempotent close() methods.

Therefore it's both safe to have them both handled by ARM, and (I'd
argue) clearer to do so, as it's then clear both objects _do_ get
closed, without having to consider the finer details of
ZipOutputStream.close().

I believe Joe was defining the considerations needed when making such a
modification in the general case.

(Joe, please correct me if I misinterpreted this).


That is correct; I was noting some subtle considerations for the more 
general case.


When both resources are java.io.Closeable, the most robust pattern is to 
have one resource variable declared for each resource.


-Joe

Re: Request for Review of 7116890 (Warning Cleanup java.io)

[Joseph Darcy]

On 12/1/2011 4:22 PM, Stuart Marks wrote:

On 12/1/11 2:13 PM, Stuart Marks wrote:

On 12/1/11 12:38 PM, Alan Bateman wrote:

On 01/12/2011 18:35, Sebastian Sickelmann wrote:

:
Thanks Alan,
L67-68 was a backporting (from a more complex solution to a small 
warning

cleanup) issue. I missed the type parameters here.
I changed L119-120 also.

New webrev is here:
http://dl.dropbox.com/u/43692695/oss-patches/openjdk8/Warning_Cleanup_Java_io/CR7116890_1/index.html 





Looks fine to me.


Hi Sebastian!

The change looks fine to me too. I'll apply the patch, build it, and 
push it in.


Thanks for your contribution!


Well, turns out this patch had an error in it. :-( In 
ExpiringCache.java the constructor that takes a long parameter has 
code that looks like this:


map = new LinkedHashMap<>() { ... }

It turns out that it's illegal to use diamond in the construction of 
an anonymous class! Didn't you compile the code, Sebastian? :-) To be 
fair, neither of us reviewers caught this either. The compiler is the 
ultimate reviewer in this case.


FYI, the reason diamond cannot be used for anonymous class constructors 
is that doing so would require a change the the JVM signature attribute, 
a de factor JVM change that was out of scope for Project Coin.


-Joe