[13]RFR 8222969 : Migrate RuleBasedCollatorTest to JDK Repo

[Ying Zhou]

Hello,

Please help to review this patch for migrating RuleBasedCollatorTest 
from Tong to Jtreg.


Bug: https://bugs.openjdk.java.net/browse/JDK-8222969

webrev: http://cr.openjdk.java.net/~yzhou/8222969/webrev.03/

Thanks,

Daisy

RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

Hello!

Jdk has several places with similar logic:  an array needs to be 
reallocated (by at least some defined amount), taking into account the 
maximum allowed size of arrays.


There's clearly an opportunity for refactoring, so it is proposed to 
introduce a dedicated utility method for calculating the best new size 
of an array.


Would you please help review this enhancement?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8223593
WEBREV: http://cr.openjdk.java.net/~igerasim/8223593/00/webrev/

Mach5 job ran fine.

Thanks in advance!

--
With kind regards,
Ivan Gerasimov

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

[Stuart Marks]

On 5/7/19 4:33 PM, Brent Christian wrote:

Collection.java:

  110  * that use different membership semantics. For operations that involve 
more than
  111  * one collection, it is specified which collection's membership semantics 
are

  112  * used by the operation.

addAll() and copyOf() involve more than one collection, though I agree that they 
do not need to be updated to specify membership semantics.


Yeah. Maybe I can be more specific here... operations that involve membership in 
both collections, or something.



AbstractCollection.java:


404 * obtaining an iterator from the {@code iterator} method. Each element
405 * is passed to the {@code contains} method of the specified collection.
406 * If this call returns {@code false}, the element is removed from
  ^

Is "this call" a little ambiguous?  Maybe:

"If contains() returns false..."
or
"If false is returned..."

?


Yeah I agree it's a bit clumsy. I'll tweak it to be better.


List.java:

Should containsAll(), removeAll(), retainAll() have the @implNote about 
contains() performance?


Good question.

I started to go and add them, but then I thought that it didn't make sense. 
List.contains() is almost unavoidably linear in performance -- at least our 
built-in List implementations are -- so the note on containsAll() is mostly 
pointless.


However, the bulk removals (removeAll and retainAll) on ArrayList are optimized 
to make a single pass over the list elements (i.e., they don't do repeated 
copying) so the performance of contains() on the specified collection is indeed 
relevant. I'll add the notes to those methods.


s'marks

Re: inconsistency between ArrayList.modCount and logic of ArrayList::addAll

[Stuart Marks]

On 5/8/19 7:44 AM, Pavel Rappo wrote:

We will certainly hear from Stuart on this very soon.


Heh.

On 5/8/19 5:55 AM, Сергей Цыпанов wrote:
javadoc for AbstractList.modCount is described as 


The number of times this list has been structurally modified.
Structural modifications are those that change the size of the
list, or otherwise perturb it in such a fashion that iterations in
progress may yield incorrect results.


However when we execute this


ArrayList objects = new ArrayList<>();
boolean result = objects.addAll(Collections.emptyList());


modCount is 1, not 0 while result is false. I.e. returned value claims the list 
is not modified, but the inner state of the same list demonstrates the opposite.


I think Pavel has the right point here, which is whether modCount should be 
incremented if the list is *actually* modified or whether an *attempt* to modify 
it is made. On the one hand, a strict reading seems to indicate incrementing 
only actual modifications. However, if the goal is to catch programming errors, 
then incrementing on any modification *attempt* seems preferable.


I described this as "state-dependent" behavior in a message in this recent 
thread:

https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059708.html
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/059979.html

Briefly, consider code like

for (Object obj : objects) {
...
objects.addAll(getAdditionalObjects());
...
}

This is clearly bad code because it modifies the list during iteration. We want 
ConcurrentModificationException to be thrown regardless of whether 
getAdditionalObjects() returns an empty collection.


In general, the incrementing of modCount (and the consequential throwing of CME) 
is an approximation. We usually want to it catch as many errors as feasible, but 
not at the cost of unreasonable performance or code complexity.


s'marks

Re: RFR(trivial): 8222394: HashMap.compute() throws CME on an empty Map if clear() called concurrently

[Stuart Marks]

On 5/4/19 12:21 AM, Patrick Zhang OS wrote:

Thank you very much for the detailed explanation and examples, which solved most of my 
previous questions and confusion. Really appreciate that. I agree with the opinion about 
"effort vs benefit".

Re-thought my original tests concerning CMEs that passed with jdk8u but failed with jdk11 (should be 9+, but I only 
checked LTS builds), I was struggling between "fixing" the "problems" in jdk11 and "making 
jdk8 fail similarly". However so far it looks these tests might be over strict, or "verifying CMEs" 
itself might be an invalid (or unreliable) operation, perhaps I should drop all of them. Lastly, a suggestion would be: 
adding more comments for this in case anyone else would revisit it with similar confusions, e.g. HashMap.clear.


Great, I'm glad this was helpful.

Regarding the tests, yes, I'm afraid those might be overly strict. The 
specification is admittedly quite loose in this area. This means that the exact 
behavior may vary from release to release. If a collection is optimized, for 
example, sometimes it's quite difficult to preserve the exact same behavior in 
all cases. It's for this reason we avoid specifying the exact behaviors around 
CME. Of course, when making any changes, we usually do try to preserve the 
general behavior, just not every specific edge case.


I generally welcome code comments to improve clarity, but I'm not sure one is 
warranted for HashMap.clear(). The placement of modCount++ seems quite 
reasonable. There are other cases where the choice of 
conditional-vs-unconditional is made (see [1]) and I don't think we want to go 
sprinkling explanations of this all around the code.


[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060068.html

s'marks

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

[Stuart Marks]

On 5/7/19 8:21 AM, Pavel Rappo wrote:

That looks good.

A tiny note on your FIXME comment [1]:

   68 // FIXME: the first paragraph below is copied from Set.equals, because
   69 // {@inheritDoc} will include text from Object.equals.

You might want to check if it is the same as [2] or [3]. If it is the same,
consider adding a reference to the appropriate issue to that FIXME comment for
future maintainers. It looks like there are some problems when inheriting
documentation from methods defined in java.lang.Object.



[1] 
http://cr.openjdk.java.net/~smarks/reviews/6394757/webrev.0/src/java.base/share/classes/java/util/AbstractSet.java.sdiff.html
[2] https://bugs.openjdk.java.net/browse/JDK-8144034
[3] https://bugs.openjdk.java.net/browse/JDK-6376959


Thanks Pavel. I looked at this and it does seem related to javadoc bug 
JDK-6396959. I've added some notes to that bug report. I'll add a reference to 
this bug report in the changeset near FIXME.


There are a couple other places in the collections doc that have this same 
pathology (i.e., text is copied instead of @inheritDoc). I'll see if I can find 
them and note them with a FIXME as well.


Thanks also for your misc markup changes sent offline.

s'marks

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

[Stuart Marks]

On 5/3/19 6:20 AM, Tagir Valeev wrote:

I'm not a Reviewer, but strongly support this change. Simpler is better. Thanks!


[capitalization adjusted]

Hi Tagir,

Thanks for looking at this! I appreciate the review.

As an aside for other readers, there's no requirement that one be a (capital-R) 
Reviewer in order to review code or changes. If you see a mistake or some issue, 
please point it out, even if you're not a Reviewer. This can be a useful 
contribution. The requirement for integration is that there be a review from 
least one Reviewer. There can be any number of reviews from non-Reviewers.


s'marks

Re: RFR(JDK 13/java.xml) 8222991: Xerces 2.12.0: Validation

[Lance Andersen]

Hi Joe,

Sorry for the delay.  The patch looks good.

Best
Lance
> On Apr 29, 2019, at 6:03 PM, Joe Wang  wrote:
> 
> Please review an update on the Validation implementation.  All XML tests and 
> JCK passed.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8222991
> webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8222991/webrev/
> 
> Thanks,
> Joe
> 

 
  

 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: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

On 5/7/2019 4:39 PM, Roger Riggs wrote:

Hi,

Additional comments/observations on the jpackage webrev.
Cleanup opportunities for more maintainable code.


When using the ToolProvider API in an application that has a security 
manager,

what permissions must be available?  Properties, files, ...?


Main:

 91: "processArguments" seems misnamed for the method that does 
*everything*.
I quite agree. Perhaps this can be done in conjunction with JDK-8223322 
 which will revise the 
flow of Main and ToolProvider.
(adding comment in JDK-8223322) 



BundleParams:

 - 116: Why type-variable "C" instead of the conventional T?
I don't know the history of this, but it seems reasonable to change to 
"T" like other classes.
(added as a line item 19 in JDK-8223241 
)


StandardBundlerParam:

 - Many unused imports (according to IntelliJ) including some in the 
same jdk.jpackage.internal that are not needed.
already listed as item 1 in JDK-8223241 



138: TODO and typo in comment.

remove todo - this has been tested
(added line item 20 in JDK-8223241 
)


148: when does the pathSep get replaced with spaces?  Will that cause 
an issue in re-parsing the string?


661-662: escaped is never set to true,  so escaping is not supported?

need to look into this - (added to JDK-8223334)


747: System.getProperty can require permissions to get properties if 
run under a security manager.
Many of the operations performed by jpackage too would require 
all-permissions if run under security manager.


CLIHelp:
 - 58, 65, 72, 80: Indentation of pLaunchOptions does not line up.

I don't see what you mean here.  Looks lined up to me


IOUtils:
 - 262: why the mix of ProcessBuilder and Runtime.exec  - stick to 
ProcessBuilder

added to this case to JDK-8223334


Log:
   "JPACKAGE_DEBUG" environment variable - ? uppercase, documented?

implemented as strictly upper case, what do we have to do to document ?


ValidOptions: 46: typo in comment:  "in the a"
several problems with this comment - added to JDK-8223241 



VersionExtractor.java seems to be dead code.

Dead code in RelativeFileset:  upshift(), contains(), copy constructor().

Platform.java:
 - should use System Runtime.Version class.

Params.java: Dead code  does not seem to be used in DeployParams
  (and would probably be better as a map than an individual object).
   setParams is unused.
new JBS issue to remove dead code. (JDK-8223586 
)




DeployParams:

 - If there are accessor methods, use them!

 - Lots of unused methods. (According to IntelliJ)

 - setSystemWide should use primitive boolean, not Boolean; 
conversions will be done as needed.


 - Ditto installDirChooser

 - Ditto SignBundle flag

Many .java files that pass a map of parameters use the argument name 
"p" but using "params"
would communicate better and be more consistent across the different 
files.


LinuxDebBundler and LinuxRpmBundler should share more code.

MacAppBundler.java:
 - list of categories will need maintenance to stay in sync with 
Apple.  Can the list be derived from the installed Mac tool chain?
 - or don't validate the argument until it is built and let the mac 
app builder do the checking.


All above added to JDK-8223586 



/Andy


Regards, Roger


On 04/29/2019 05:58 PM, Andy Herrick wrote:

jpackage reviewers:

We hope to move JEP 343 to "Proposed to Target" next week, so we 
would like expedite the review process as much as possible.

Re: [13] RFR: 8221432: Upgrade CLDR to Version 35.1

[Roger Riggs]

Hi Naoto,

The changes look fine.

Thanks, Roger

On 05/07/2019 04:12 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8221432/webrev.02/

The webrev is big, but the large amount of the changes is simply 
replacing the CLDR data files from v33 to v35.1, which includes the 
translations for the Japanese new era, Reiwa.


Naoto

Re: [13] RFR: 8221432: Upgrade CLDR to Version 35.1

[Steven R. Loomis]

Hi Naoto,
  The comment in 
http://cr.openjdk.java.net/~naoto/8221432/webrev.02/make/jdk/src/classes/build/tools/cldrconverter/Bundle.java.udiff.html
 "fix up 'Reiwa' era, which can be missing in some locales” seems to imply that 
fallback resolution isn’t working properly, because that data (R) is present in 
root.xml.

 The deleted comment in the same file, "Some locale data has no default script 
for numbering even with mutiple scripts” is not true for the same reason.

 Otherwise the changes look as expected on a spot check.

--
Steven R. Loomis | @srl295 | git.io/srl295



> El may. 7, 2019, a las 1:12 p. m., naoto.s...@oracle.com escribió:
> 
> Hello,
> 
> Please review the fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8221432
> 
> The proposed changeset is located at:
> 
> http://cr.openjdk.java.net/~naoto/8221432/webrev.02/
> 
> The webrev is big, but the large amount of the changes is simply replacing 
> the CLDR data files from v33 to v35.1, which includes the translations for 
> the Japanese new era, Reiwa.
> 
> Naoto

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[coleen . phillimore]

On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:

Hi,

Please have a look at this further improved webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-incremental/

Harold, Coleen, thanks for pointing me to those methods.
I'm using them now. This simplifies the helper methods
considerably.

Ralf, thanks for looking at the messages!
I now suppress the "static " and
"The return value of '" strings in the array subscript
expressions and added corresponding test cases.

About "can not" versus "cannot", what I find in the
net "cannot" is to be preferred.  Any comments on that?
By native speakers?
See example messages here:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/output_with_debug_info.txt


Cannot is apparently preferable in English.  Native speaker (only 
language) but somebody had to tell me.


Coleen


Further, I fixed a build issue with the solaris compiler.
All handling of bci is now unsigned.

Best regards,
   Goetz.





-Original Message-
From: Schmelter, Ralf
Sent: Dienstag, 7. Mai 2019 14:35
To: Lindenmaier, Goetz ; Java Core Libs ; hotspot-runtime-...@openjdk.java.net; Coleen
Phillimore (coleen.phillim...@oracle.com) 
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

Hi Goetz,

I've played with the messages a little bit and they generally look good. But 
I've
come across two which look strange:
  - 'o[static Test.INDEX]' is null. Can not invoke method 'int
java.lang.Object.hashCode()'
  - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not 
invoke
method 'int java.lang.Object.hashCode()'.

Here is the test program to reproduce these:
public class Test {
 public static int INDEX = 2;

 public static void main(String[] args) {
 Object[] o = new Object[10];
 try {
  o[INDEX].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 try {
  o["".hashCode()].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
}

And 'Can not' should be 'Cannot'?

Best regards,
Ralf

RFR: 8223454: Reduce String concatenation shapes by folding initialLengthCoder into last mixer

[Claes Redestad]

Hi,

yet another String concat startup/footprint optimization.

As the subject implies, we could fold the initialLengthCoder it into the
final mixer. This way we end up with one less bound argument into the
larger method handle combinator tree, with fewer species classes and
lambda forms generated as a result. This means a significant speed-up on
several startup and footprint tests, and especially noticeable on
realistic workloads that just have a few different small concat shapes.

Bug:https://bugs.openjdk.java.net/browse/JDK-8223454
Webrev: http://cr.openjdk.java.net/~redestad/8223454/open.00/

Thanks!

/Claes

Re: inconsistency between ArrayList.modCount and logic of ArrayList::addAll

[Pavel Rappo]

We will certainly hear from Stuart on this very soon. Meanwhile, I have to say
that the situation you've described is not that clear-cut in my opinion.

I understand there might be a discrepancy between how the `modCount` field is
described and how it is actually used.

Incrementing `modCount` unconditionally makes more sense to me. The whole
purpose of this mechanism is to fail fast, exposing possible bugs in the client
code. I believe it should be incremented on an attempt to modify the collection,
rather than on the effective result of that attempt.

It's somewhat similar to the following question: should Collections.emptyList
(or Collections.emptySet) throw an exception on an attempt to call `clear` (or
`remove(e)` where `e` is not a member of the said collection)?

-Pavel

Re: RFR: 8221340 - [TESTBUG] TestCgroupMetrics.java fails after fix for JDK-8219562

[Bob Vandette]

Thanks Misha!

Could I get a “Review” from someone in core libs, please?

Bob.

> On May 7, 2019, at 8:49 PM, mikhailo.seledt...@oracle.com wrote:
> 
> Changes look good to me,
> 
> Misha
> 
> 
> On 5/7/19 5:56 AM, Bob Vandette wrote:
>> Please review this change to the Container Metrics implementation.
>> 
>> Change were made in JDK-8219562 [1] to osContainer_linux.cpp to correct
>> the parsing of of the /proc/self/mountinfo file but corresponding changes to 
>> the
>> Metrics API and Container tests shou,d have been done to match this change.
>> 
>> BUG:
>> https://bugs.openjdk.java.net/browse/JDK-8221340
>> 
>> WEBREV:
>> http://cr.openjdk.java.net/~bobv/8221340/webrev.00/
>> 
>> TESTING:
>> These changes were verified by running Mach5  tier1 and 2 tests and the 
>> jtreg container
>> tests on a Linux x64 system.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8219562
>> 
>> Bob.
>> 
> 

inconsistency between ArrayList.modCount and logic of ArrayList::addAll

[Сергей Цыпанов]

Hello,

javadoc for AbstractList.modCount is described as 

> The number of times this list has been structurally modified.
> Structural modifications are those that change the size of the
> list, or otherwise perturb it in such a fashion that iterations in
> progress may yield incorrect results.

However when we execute this


ArrayList objects = new ArrayList<>();
boolean result = objects.addAll(Collections.emptyList());


modCount is 1, not 0 while result is false. I.e. returned value claims the list 
is not modified, but the inner state of the same list demonstrates the opposite.

The reason is implementation of List::addAll method:

public boolean addAll(Collection c) {
Object[] a = c.toArray();
modCount++; 
// <--
int numNew = a.length;
if (numNew == 0)
return false;   
 // <--
Object[] elementData;
final int s;
if (numNew > (elementData = this.elementData).length - (s = size))
elementData = grow(s + numNew);
System.arraycopy(a, 0, elementData, s, numNew);
size = s + numNew;
return true;
}

I think modCount++;  should be placed after value of numNew is checked. Then 
it's incremented only when underlying array is actually changed.

Regards,

Sergei Tsypanov

Re: RFR: JDK-8222930: ConcurrentSkipListMapTest.clone() broken since jdk10

[Adam Farley8]

Good job. :)

On to the next bug! Hehe.

Best Regards

Adam Farley 
IBM Runtimes


Martin Buchholz  wrote on 07/05/2019 19:46:26:

> From: Martin Buchholz 
> To: Adam Farley8 
> Cc: Java Core Libs 
> Date: 07/05/2019 19:46
> Subject: Re: RFR: JDK-8222930: ConcurrentSkipListMapTest.clone() 
> broken since jdk10
> 
> Thanks!  Fix is now committed to jdk11, jdk12, jdk13.
> 
> From: Adam Farley8 
> Date: Mon, Apr 29, 2019 at 3:16 AM
> To: 
> Cc: Java Core Libs

> Hi All, 
> 
> Reviews and feedback requested for the fix. 
> 
> http://cr.openjdk.java.net/~afarley/8222930.1/jdk13/webrev 
> 
> Martin: Thanks for the testcase. I've replaced the old test in the 
> webrev with your generalized one. :) 
> 
> Best Regards
> 
> Adam Farley 
> IBM Runtimes
> 
> 
> Adam Farley8/UK/IBM wrote on 25/04/2019 13:47:13:
> 
> > From: Adam Farley8/UK/IBM 
> > To: Stuart Marks  
> > Cc: Java Core Libs  
> > Date: 25/04/2019 13:47 
> > Subject: Re: RFR: JDK-8222930: ConcurrentSkipListMapTest.clone() 
> > broken since jdk10 
> > 
> > Hi Stuart, 
> > 
> > Whoops, typo. Thanks for catching that. 
> > 
> > Ditto for Martin, who has modified the bug. :)
> > 
> > Best Regards
> > 
> > Adam Farley 
> > IBM Runtimes
> 
> > 
> > Stuart Marks  wrote on 24/04/2019 17:59:17:
> > 
> > > From: Stuart Marks  
> > > To: Adam Farley8  
> > > Cc: Java Core Libs  
> > > Date: 24/04/2019 17:59 
> > > Subject: Re: RFR: JDK-8222930: ConcurrentSkipListMapTest.clone() 
> > > broken since jdk10 
> > > 
> > > Hi Adam,
> > > 
> > > Thanks for finding this bug!
> > > 
> > > This is a bug in ConcurrentSkipListMap itself, not some test named 
> > > ConcurrentSkipListMapTest. I'd suggest changing the bug summary 
> > line and the 
> > > commit message accordingly.
> > > 
> > > Thanks,
> > > 
> > > s'marks
> > > 
> > > On 4/24/19 9:20 AM, Adam Farley8 wrote:
> > > > ConcurrentSkipListMapTest.clone() produces a clone that sharesthe 
array
> > > > size variable of the original, and then doubles it.
> > > > 
> > > > So both arrays, original and clone, tell the user that each is 
twice as
> > > > big as it actually is.
> > > > 
> > > > The proposed fix is to simply set the clone's array size 
> variable to null
> > > > during creation.
> > > > 
> > > > Fix and test code available.
> > > > 
> > > > Reviews and sponsor requested.
> > > > 
> > > > Webrev: https://urldefense.proofpoint.com/v2/url?
> > > u=http-3A__cr.openjdk.java.net_-7Eafarley_8222930.
> > > 0_jdk13_webrev_=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=P5m8KWUXJf-
> > > 
> > 
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=9_BHLxc2OwO4OJABunATso0Ej3_keQ0c5uQJZ4AwSfk=0gBgd8gUhNlM26eNWxBbpnIAsFJPwnOtsmT6qH72NPM=
> > > > 
> > > > Bug: https://urldefense.proofpoint.com/v2/url?
> > > 
> > 
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8222930=DwICaQ=jf_iaSHvJObTbx-
> > > siA1ZOg=P5m8KWUXJf-
> > > 
> > 
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=9_BHLxc2OwO4OJABunATso0Ej3_keQ0c5uQJZ4AwSfk=vNk7C72hr8FqiYLJEVvCR69vlhPuT7zSIAiJ9Tl91JQ=
> > > > 
> > > > Best Regards
> > > > 
> > > > Adam Farley
> > > > IBM Runtimes
> > > > 
> > > > P.S. Apparently this has been broken since JDK 10, so we should 
look at
> > > > backporting (at least to 11 and 12) once this is in.
> > > > 
> > > > Unless stated otherwise above:
> > > > IBM United Kingdom Limited - Registered in England and Wales with 
number
> > > > 741598.
> > > > Registered office: PO Box 41, North Harbour, Portsmouth, 
> Hampshire PO6 3AU
> > > > 
> > > 
> > 
> > Unless stated otherwise above:
> > IBM United Kingdom Limited - Registered in England and Wales with 
> > number 741598. 
> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 
3AU
> 
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with 
> number 741598. 
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 
3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

RFR: 8223553: Fix code constructs that do not compile with the Eclipse Java Compiler

[Langer, Christoph]

Hi,

please review a small change that I'd like to see in OpenJDK to get rid of 
compilation errors in the Eclipse IDE.

It seems the root cause for the compilation errors is that javac would 
sometimes widen capture variables and is hence able to compile the places that 
I touch here. The EC4J compiler claims that it's following the spec more 
strictly and bails out at these places. I had posted about this on compiler-dev 
but got no reaction [0].

So, as this seems to be some field of open work for the compiler/spec folks, 
I'd like to get EC4J quiesced by introducing some slight modifications to the 
original places which makes the code appeal a bit less elegant and fluent but 
will get rid of the compile errors.

Please review:
Bug: https://bugs.openjdk.java.net/browse/JDK-8223553
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8223553.0/

The modification to 
src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java 
belongs to JSR-166, so I don't know if it needs some special handling.

Thanks & Best regards
Christoph

[0] https://mail.openjdk.java.net/pipermail/compiler-dev/2019-March/013054.html

Re: RFR: 8212700: Change the mechanism by which JDK loads the platform-specific AWT Toolkit

[Sergey Bylokhov]

OK, looks fine then.


On 5/7/19, 7:47 PM, Sergey Bylokhov wrote:

Hi, Phil.

Looks fine, but probably it will be worth to rename PlatformGraphicsInfo to 
PlatformInfo?


I don't want (or intend) to do that. I already knew I was going
to re-use this class for this purpose when I created it and adjusted
the classname then to be reasonably acceptable. There is no ideal
and PlatformInfo isn't it either.

-phil.



On 06/05/2019 15:11, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8212700
CSR: https://bugs.openjdk.java.net/browse/JDK-8223417
Webrev: https://cr.openjdk.java.net/~prr/8212700

Please review this bug + CSR to remove the awt.headless system property.

This should be the last of the fixes to remove use of system
properties set in the launcher to specify per-platform
internal classes and/or behaviours of the java.desktop module.

Half of the change is removing code from the launcher, and
then some small changes in Toolkit.java to consult a new
method in the per-platform internal PlatformGraphicsInfo.java class.

However it was also necessary to update each of the platform toolkit class
constructor code since they all get/set a system property to support
extra mouse buttons (I don't know why this could not be shared in
SunToolkit but that is another matter than this fix), and two of them
access the root thread group. Previously they inherited privileges
from the code in java.awt.Toolkit which instantiated them but now
they need to assert the privileges directly - at to the point of use.

As well as adding a simple new regression test, all the automated
headless tests and many automated headful tests were run as well
as basic manual testing.

The CSR also covers updating the javadoc of Toolkit.getDefaultToolkit()

-phil






--
Best regards, Sergey.