Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message
Hi Martin, I'd appreciate it if you could take a look at this. Thanks, s'marks On 12/3/20 10:57 PM, Stuart Marks wrote: This rewrites the doc of ArraysSupport.newLength, adds detail to the exception message, and adds a test. In addition to some renaming and a bit of refactoring of the actual code, I also made two changes of substance to the code: 1. I fixed a problem with overflow checking. In the original code, if oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this method could return a negative value. It turns out that writing tests helps find bugs! 2. Under the old policy, if oldLength and minGrowth required a length above SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would return Integer.MAX_VALUE. That doesn't make any sense, because attempting to allocate an array of that length will almost certainly cause the Hotspot to throw OOME because its implementation limit was exceeded. Instead, if the required length is in this range, this method returns that required length. Separately, I'll work on retrofitting various call sites around the JDK to use this method. - Commit messages: - 8247373: ArraysSupport.newLength doc, test, and exception message Changes: https://git.openjdk.java.net/jdk/pull/1617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1617=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247373 Stats: 171 lines in 4 files changed: 137 ins; 3 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/1617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617 PR: https://git.openjdk.java.net/jdk/pull/1617
RFR: 8247373: ArraysSupport.newLength doc, test, and exception message
This rewrites the doc of ArraysSupport.newLength, adds detail to the exception message, and adds a test. In addition to some renaming and a bit of refactoring of the actual code, I also made two changes of substance to the code: 1. I fixed a problem with overflow checking. In the original code, if oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this method could return a negative value. It turns out that writing tests helps find bugs! 2. Under the old policy, if oldLength and minGrowth required a length above SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would return Integer.MAX_VALUE. That doesn't make any sense, because attempting to allocate an array of that length will almost certainly cause the Hotspot to throw OOME because its implementation limit was exceeded. Instead, if the required length is in this range, this method returns that required length. Separately, I'll work on retrofitting various call sites around the JDK to use this method. - Commit messages: - 8247373: ArraysSupport.newLength doc, test, and exception message Changes: https://git.openjdk.java.net/jdk/pull/1617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1617=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247373 Stats: 171 lines in 4 files changed: 137 ins; 3 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/1617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617 PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > This patch replaces some uses of `Reference::get` to `Reference::refersTo` to > avoid keeping a referent strongly reachable that could cause unnecessary > delay in collecting such object. I only made change in some but not all > classes in core libraries when working with Kim on `Reference::refersTo`. > The remaining uses are left for the component owners to convert at > appropriate time. Hi Mandy, Looks good. I guess, you are going to update the copyright comments before the push. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Thu, 3 Dec 2020 14:44:05 GMT, Marius Volkhart wrote: >> @MariusVolkhart Here is a PR for your branch so the test works with jtreg: >> https://github.com/MariusVolkhart/jdk/pull/1 >> >> Fails before the JDK patch and passes after. >> >> Run it with: >> $ rm -rf JTwork JTreport && jtreg >> -jdk:./build/linux-x86_64-server-release/images/jdk -verbose:summary >> test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java >> Passed: javax/xml/jaxp/8256515/XmlInputFactoryTest.java >> Test results: passed: 1 >> Results are in *.jtr files: >> >> $ find JTwork/ -name *.jtr >> JTwork/javax/xml/jaxp/8256515/XmlInputFactoryTest.jtr >> >> Or using the test framework of OpenJDK with: >> >> $ bash configure --with-jtreg=/path/to/jtreg [...] >> $ make test TEST="jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java" >> >> HTH > > @jerboaa Thanks! That PR was immensely helpful, not just in helping get this > change moving, but in helping me understand how to write the tests for next > time! When you update, pls do a merge from the latest upstream jdk master. The changeset is pretty far behind the master. - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
On Thu, 3 Dec 2020 23:07:25 GMT, Ivan Šipka wrote:
>> Ivan Šipka has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8166026: Refactor java/lang shell tests to java
>
> test/jdk/java/lang/Thread/UncaughtExceptionsTest.java line 94:
>
>> 92: final static String EXPECTED_RESULT = "OK";
>> 93:
>> 94: public static void seppuku() { throw new
>> RuntimeException("Seppuku!"); }
>
> @RogerRiggs I suggest method name instead of `seppuku` to be
> `throwRuntimeException`?
> And to remove the message from the exception constructor.
Both suggestions work for me. Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/1578
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
On Thu, 3 Dec 2020 19:37:11 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test.
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
> Please rename the class Seppuku to a more generic name for a process that
> exits as the result of an uncaught exception. Perhaps a simple "UncaughtExit".
How does `UncaughtExitSimulator` sound?
test/jdk/java/lang/Thread/UncaughtExceptionsTest.java line 94:
> 92: final static String EXPECTED_RESULT = "OK";
> 93:
> 94: public static void seppuku() { throw new
> RuntimeException("Seppuku!"); }
@RogerRiggs I suggest method name instead of `seppuku` to be
`throwRuntimeException`?
And to remove the message from the exception constructor.
-
PR: https://git.openjdk.java.net/jdk/pull/1578
RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
This patch replaces some uses of `Reference::get` to `Reference::refersTo` to avoid keeping a referent strongly reachable that could cause unnecessary delay in collecting such object. I only made change in some but not all classes in core libraries when working with Kim on `Reference::refersTo`.The remaining uses are left for the component owners to convert at appropriate time. - Commit messages: - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - 8256167: Convert JDK use of to - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo - minor cleanup - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into refersTo - improve refersTo0 descriptions - basic functional test - change referent access - expand test - ... and 2 more: https://git.openjdk.java.net/jdk/compare/f0b11940...b1e645b1 Changes: https://git.openjdk.java.net/jdk/pull/1609/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1609=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256167 Stats: 52 lines in 12 files changed: 7 ins; 10 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/1609.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609 PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8256894: define test groups [v3]
> Defined new test groups as defined in ticket. @fguallini Ivan Šipka has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8257516: define test group for manual tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/1416/files - new: https://git.openjdk.java.net/jdk/pull/1416/files/c91558be..c47bb096 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1416=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1416=01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1416/head:pull/1416 PR: https://git.openjdk.java.net/jdk/pull/1416
Integrated: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
On Thu, 3 Dec 2020 19:10:46 GMT, Mandy Chung wrote: > VarHandleTestByteArrayAsInt.java intermittently fails only fastdebug build > and slow mac os machines. I can't reproduce this locally. It fails on jdk > 16 internal testing on a few builds. I propose to increase the timeout and > see if that is adequate or not. We will re-examine this test if the > increased timeout is not adequate. This pull request has now been integrated. Changeset: f0b11940 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/f0b11940 Stats: 28 lines in 7 files changed: 0 ins; 0 del; 28 mod 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits Reviewed-by: bchristi, naoto - PR: https://git.openjdk.java.net/jdk/pull/1603
Re: RFR: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
On Thu, 3 Dec 2020 19:10:46 GMT, Mandy Chung wrote: > VarHandleTestByteArrayAsInt.java intermittently fails only fastdebug build > and slow mac os machines. I can't reproduce this locally. It fails on jdk > 16 internal testing on a few builds. I propose to increase the timeout and > see if that is adequate or not. We will re-examine this test if the > increased timeout is not adequate. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1603
Re: RFR: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
On Thu, 3 Dec 2020 19:10:46 GMT, Mandy Chung wrote: > VarHandleTestByteArrayAsInt.java intermittently fails only fastdebug build > and slow mac os machines. I can't reproduce this locally. It fails on jdk > 16 internal testing on a few builds. I propose to increase the timeout and > see if that is adequate or not. We will re-examine this test if the > increased timeout is not adequate. Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1603
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
On Thu, 3 Dec 2020 19:33:14 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
Changes requested by rriggs (Reviewer).
test/jdk/java/lang/annotation/LoaderLeakTest.java line 65:
> 63: @Test
> 64: public void testWithoutReadingAnnotations() throws Throwable {
> 65: runJavaProcessExpectSuccesExitCode("Main");
Succes -> Success typo. *Everywhere*
test/jdk/java/lang/annotation/LoaderLeakTest.java line 133:
> 131: class SimpleClassLoader extends ClassLoader {
> 132:
> 133: private Hashtable classes = new Hashtable();
A good upgrade would be to use HashMap instead of the ancient
HashTable.
Also, avoid Raw types and cleanup other compile time warnings.
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
On Thu, 3 Dec 2020 19:37:11 GMT, Ivan Šipka wrote: >> Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test. > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java Please rename the class Seppuku to a more generic name for a process that exits as the result of an uncaught exception. Perhaps a simple "UncaughtExit". - Changes requested by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1578
Re: RFR: 8166026: refactor shell tests to java [v3]
On Thu, 3 Dec 2020 19:46:14 GMT, Ivan Šipka wrote: >> @iignatev could you please review? Thank you. >> >> note to self: >> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java >> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java >> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java >> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Sat, 28 Nov 2020 08:42:52 GMT, John Lin
wrote:
>> @johnlinp In any case, you'd also need to update the surrounding prose; we
>> need to remove this:
>> returning the current value or {@code null} if absent:```
>
> @pavelrappo Please see my updated CSR below. Thanks.
>
> # Map::compute should have the implementation requirement match its default
> implementation
>
> ## Summary
>
> The implementation requirement of Map::compute does not match its default
> implementation. Besides, it has some other minor issues. We should fix it.
>
> ## Problem
>
> The documentation of the implementation requirements for Map::compute has the
> following problems:
> 1. It doesn't match its default implementation.
> 1. It lacks of the return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
> 1. The surrounding prose contains incorrect statements.
>
> ## Solution
>
> Rewrite the documentation of Map::compute to match its default implementation
> and solve the above mentioned problems.
>
> ## Specification
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..b30e3979259 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1107,23 +1107,17 @@ public interface Map {
> *
> * @implSpec
> * The default implementation is equivalent to performing the following
> - * steps for this {@code map}, then returning the current value or
> - * {@code null} if absent:
> + * steps for this {@code map}:
> *
> * {@code
> * V oldValue = map.get(key);
> * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * map.remove(key);
> - * } else {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null || map.containsKey(key)) {
> + * map.remove(key);
> * }
> + * return newValue;
> * }
> *
> * The default implementation makes no guarantees about detecting if
> the
The current snippet proposed by @johnlinp does seem to have the same behavior
as the default implementation; I would avoid trying to "optimize" this.
However, it does express the conditions and return value somewhat differently
from the way the default implementation does. I think those differences are not
significant to subclasses and are mostly stylistic. The original `@implSpec`
snippet attempted to handle the cases separately, whereas the current proposed
snippet minimizes them (while still agreeing with the implementation's
behavior). I'm not too concerned about this. I think the current snippet is
acceptable. Again, the main priority is agreement with the implementation.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Mon, 30 Nov 2020 15:08:51 GMT, Pavel Rappo wrote:
>> @johnlinp, thanks for updating the CSR draft; it is much better now.
>>
>> @stuart-marks, I think we could further improve this snippet. This `if`
>> statement seems to use an optimization:
>>
>> if (oldValue != null || map.containsKey(key))
>>
>> I don't think we should include an optimization into the specification
>> unless that optimization also improves readability. Is this the case here?
>> Could this be better?
>>
>> if (map.containsKey(key))
>
> I would even go as far as to rewrite that snippet like this:
>
> if (newValue == null) {
> remove(key);
> } else {
> put(key, newValue);
> }
> return newValue;
>
> This rewrite is possible thanks to the following properties of
> `Map.remove(Object key)`:
>
> 1. A call with an unmapped `key` has no effect.
> 2. A call with a mapped `key` has the same semantics regardless of the value
> that this key is mapped to.
>
> In particular, (2) covers `null` values.
>
> To me, this rewrite reads better; however, I understand that readability is
> subjective and that snippets used in `@implSpec` might be subject to
> additional requirements.
@pavelrappo The intended effect of the revised snippet is sensible, but again I
note that it differs from the actual default implementation. Specifically: if
the map does not contain the key and newValue is null, the default
implementation currently does nothing, whereas the revised snippet calls
`remove(key)`. This should have no effect _on the map_ but a subclass might
override `remove` and this behavior difference is observable. (The typical
example of this is maintaining a counter of the number of operations. I think
_Effective Java_ uses that example in discussing subclassing.) I think the main
priority here is fidelity to what the default implementation actually does --
at least, concerning operations on *this* -- and less on readability.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8166026: refactor shell tests to java [v3]
> @iignatev could you please review? Thank you. > > note to self: > jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java > test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java > test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1484/files - new: https://git.openjdk.java.net/jdk/pull/1484/files/16857464..75096d42 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1484=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1484=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1484/head:pull/1484 PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
> Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test. Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1578/files - new: https://git.openjdk.java.net/jdk/pull/1578/files/4bd57942..fa23de7f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1578=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1578=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1578/head:pull/1578 PR: https://git.openjdk.java.net/jdk/pull/1578
Integrated: 8228615: Optional.empty doc should suggest using isEmpty
On Thu, 3 Dec 2020 01:08:18 GMT, Stuart Marks wrote: > Some small doc changes. The changes are to `@apiNote` text, which is > non-normative, so no CSR is required. This pull request has now been integrated. Changeset: 2b73f992 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/2b73f992 Stats: 12 lines in 4 files changed: 0 ins; 0 del; 12 mod 8228615: Optional.empty doc should suggest using isEmpty Reviewed-by: lancea, bpb, naoto - PR: https://git.openjdk.java.net/jdk/pull/1585
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1577/files - new: https://git.openjdk.java.net/jdk/pull/1577/files/bc56a637..3a832a4d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1577=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1577=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1577.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577 PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]
> Refactor > `test/jdk/java/lang/SecurityManager/modules/CustomSecurityManager.sh` as java > test. Ivan Šipka has updated the pull request incrementally with two additional commits since the last revision: - 8166026: Refactor java/lang shell tests to java - 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1579/files - new: https://git.openjdk.java.net/jdk/pull/1579/files/6968fee1..ca546c65 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1579=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1579=00-01 Stats: 17 lines in 1 file changed: 12 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1579.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1579/head:pull/1579 PR: https://git.openjdk.java.net/jdk/pull/1579
RFR: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
VarHandleTestByteArrayAsInt.java intermittently fails only fastdebug build and slow mac os machines. I can't reproduce this locally. It fails on jdk 16 internal testing on a few builds. I propose to increase the timeout and see if that is adequate or not. We will re-examine this test if the increased timeout is not adequate. - Commit messages: - 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits Changes: https://git.openjdk.java.net/jdk/pull/1603/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1603=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8235784 Stats: 28 lines in 7 files changed: 0 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1603.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1603/head:pull/1603 PR: https://git.openjdk.java.net/jdk/pull/1603
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v3]
On Wed, 2 Dec 2020 14:40:15 GMT, Jan Lahoda wrote:
>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>>
>> From the original PR:
>>
>>> Please review the code for the second iteration of sealed classes. In this
>>> iteration we are:
>>>
>>> * Enhancing narrowing reference conversion to allow for stricter
>>> checking of cast conversions with respect to sealed type hierarchies
>>>
>>> * Also local classes are not considered when determining implicitly
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>>
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>>> still in the same method, the return type has been changed to Class[]
>>> instead of the previous ClassDesc[]
>>>
>>> * adding code to make sure that annotations can't be sealed
>>>
>>> * improving some tests
>>>
>>>
>>> TIA
>>>
>>> Related specs:
>>> [Sealed Classes
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>>
>> This PR strives to reflect the review comments from 1227:
>> * adjustments to javadoc of j.l.Class methods
>> * package access checks in Class.getPermittedSubclasses()
>> * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improving Class.getPermittedSubclasses to filter out permitted classes that
> are not a subtype of the current class, and other adjustments per the review
> feedback.
I approve this version of `Class::getPermittedSubclasses` implementation for
this PR.We need to follow up the specification of
`Class::getPermittedSubclasses` w.r.t. what it should return e.g. the classes
whatever in `PermittedSubclasses` attribute vs the classes that are permitted
subtypes at runtime and return null if this class is not sealed.
I reviewed hotspot and java.base changes (not langtools) with a couple minor
comments.
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line
51:
> 49: public class TestSecurityManagerChecks {
> 50:
> 51: private static final ClassLoader OBJECT_CL =
> Object.class.getClassLoader();
This is `null` - the bootstrap class loader. An alternative to the static
variable we can simply use null.
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line
66:
> 64: .getProtectionDomain()
> 65: .getCodeSource()
> 66: .getLocation();
This is essentially the classpath. An alternative way to get the location
through `System.getProperty("test.class.path")`.
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line
86:
> 84:
> 85: // First get hold of the target classes before we enable security
> 86: Class sealed =
> testLayer.findLoader("test").loadClass("test.Base");
I would recommend to use `Class::forName` instead of `ClassLoader::loadClass`
even though this is just a test (for security reason for example avoid type
confusion). If you want to load a class from a specific module, you can use
`Class.forName(String cn, Module m)`
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line
91:
> 89: Class[] subclasses = sealed.getPermittedSubclasses();
> 90:
> 91: if (subclasses.length != 3) {
I suggest to check against the expected list of permitted subclasses here and
also the validation in the subsequent calls to `getPermittedSubclasses` with
security manager enabled. That would help the readers easier to understand
this test.
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line
120:
> 118:
> 119: //should pass - does not return a class from package "test":
> 120: sealed.getPermittedSubclasses();
Adding a check to the expected returned value would make this clear (no
permitted subclasses of package `test`).
src/java.base/share/classes/java/lang/Class.java line 3035:
> 3033: */
> 3034: private static void
> checkPackageAccessForPermittedSubclasses(SecurityManager sm,
> 3035: final ClassLoader ccl, boolean
> checkProxyInterfaces,
`checkProxyInterfaces` parameter is not needed. It can be removed.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 09:58:16 GMT, Alan Bateman wrote: >> The attribute_length of known Module attributes in the module-info.class >> is currently ignored. It should be checked and the class rejected if the >> attribute length doesn't exactly match the length of the info in the >> attribute. >> >> There are several ways to fix this. I initially limited the reading of the >> attribute_info to the attribute length but this resulted in confusing >> exception messages as the attribute appears truncated. The exception >> messages are clearer when it checks that the attribute length corresponds to >> the number of bytes read. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - Restructure check to make it more obvious that it doesn't overflow > - Merge > - Merge > - Merge > - Trailing whitespace > - Expand test to Module attribute > - Merge > - Test cleanup > - Add test > - Merge > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/46ea739e...f15dbb1b Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v3]
On Thu, 3 Dec 2020 16:07:28 GMT, Lois Foltan wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Improving Class.getPermittedSubclasses to filter out permitted classes >> that are not a subtype of the current class, and other adjustments per the >> review feedback. > > I have reviewed the hotspot changes and they look good. > Thanks, > Lois > After a discussion with Harold, I've reverted the patch where > Class.getPermittedSubclasses returns null. Harold will do that separatelly > under JDK-8256867, unless there are objections. No objection. Keeping `Class::getPermittedSubclasses` as specified in the CSR is right thing to do so that this work can be integrated and resolve JDK-8256867 separately. - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8257622: MemoryAccess methods are missing @ForceInline annotations
On Wed, 2 Dec 2020 18:47:10 GMT, Maurizio Cimadamore
wrote:
> The accessor methods in the `MemoryAccess` class are missing `@ForceInline`
> annotations. This causes odd behavior on certain benchmarks, especially if
> these methods are called many times in the body of a single method.
Looks good with minor nit.
test/micro/org/openjdk/bench/jdk/incubator/foreign/LoopOverNonConstantFP.java
line 72:
> 70: unsafe_addrOut = unsafe.allocateMemory(ALLOC_SIZE);
> 71: for (int i = 0; i < ELEM_SIZE; i++) {
> 72: unsafe.putDouble(unsafe_addrIn + (i * CARRIER_SIZE) , i);
Here and later, excess whitespace before final comma.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1570
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v3]
On Wed, 2 Dec 2020 14:40:15 GMT, Jan Lahoda wrote: >> This pull request replaces https://github.com/openjdk/jdk/pull/1227. >> >> From the original PR: >> >>> Please review the code for the second iteration of sealed classes. In this >>> iteration we are: >>> >>> * Enhancing narrowing reference conversion to allow for stricter >>> checking of cast conversions with respect to sealed type hierarchies >>> >>> * Also local classes are not considered when determining implicitly >>> declared permitted direct subclasses of a sealed class or sealed interface >>> >>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >>> still in the same method, the return type has been changed to Class[] >>> instead of the previous ClassDesc[] >>> >>> * adding code to make sure that annotations can't be sealed >>> >>> * improving some tests >>> >>> >>> TIA >>> >>> Related specs: >>> [Sealed Classes >>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >>> [Sealed Classes >>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >>> [Additional: Contextual >>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) >> >> This PR strives to reflect the review comments from 1227: >> * adjustments to javadoc of j.l.Class methods >> * package access checks in Class.getPermittedSubclasses() >> * fixed to the narrowing conversion/castability as pointed out by Maurizio > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Improving Class.getPermittedSubclasses to filter out permitted classes that > are not a subtype of the current class, and other adjustments per the review > feedback. I reviewed the hotspot changes and they hotspot look good. Thanks, Lois - Marked as reviewed by lfoltan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 15:52:35 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 1203:
>>
>>> 1201: @Override
>>> 1202: public String readUTF() throws IOException {
>>> 1203: return DataInputStream.readUTF(this);
>>
>> If i understand correctly the code, I believe readUTF should change a
>> boolean field named `countCanNotBeTrackedAnymore` from false to true, and in
>> the method `count()`, `countCanNotBeTrackedAnymore` has to be checked and
>> throws an ISE before returning `count`
>
> Hi Rémi, I do not think that that is required. `DataInputStream.readUTF` will
> call back into `this` to do the reading so the `count` should be properly
> incremented? Or maybe I'm missing something. Best regards!
Thanks,
i should have read the code more carefully.
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 15:55:15 GMT, Rémi Forax wrote: >> Hi Rémi, I do not think that that is required. `DataInputStream.readUTF` >> will call back into `this` to do the reading so the `count` should be >> properly incremented? Or maybe I'm missing something. Best regards! > > Thanks, > i should have read the code more carefully. The module-info is read sequentially so the constant pool is read (and indexed) before the attributes are read. So any references to UTF-8 constants doesn't involve random access. But maybe you mean something else? - PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 12:50:56 GMT, Rémi Forax
wrote:
>> Alan Bateman has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 11 additional
>> commits since the last revision:
>>
>> - Restructure check to make it more obvious that it doesn't overflow
>> - Merge
>> - Merge
>> - Merge
>> - Trailing whitespace
>> - Expand test to Module attribute
>> - Merge
>> - Test cleanup
>> - Add test
>> - Merge
>> - ... and 1 more:
>> https://git.openjdk.java.net/jdk/compare/5bb86f87...f15dbb1b
>
> src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 1203:
>
>> 1201: @Override
>> 1202: public String readUTF() throws IOException {
>> 1203: return DataInputStream.readUTF(this);
>
> If i understand correctly the code, I believe readUTF should change a boolean
> field named `countCanNotBeTrackedAnymore` from false to true, and in the
> method `count()`, `countCanNotBeTrackedAnymore` has to be checked and throws
> an ISE before returning `count`
Hi Rémi, I do not think that that is required. `DataInputStream.readUTF` will
call back into `this` to do the reading so the `count` should be properly
incremented? Or maybe I'm missing something. Best regards!
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Integrated: 8257591: Remove suppression of record preview related warnings in java.lang
On Thu, 3 Dec 2020 10:39:05 GMT, Julia Boes wrote: > Records exit preview in JDK 16. This change removes preview related > suppression warnings in some source files and removes the '--enable-preview' > option for compilation and execution of some tests that use record classes. This pull request has now been integrated. Changeset: b170c837 Author:Julia Boes URL: https://git.openjdk.java.net/jdk/commit/b170c837 Stats: 16 lines in 6 files changed: 0 ins; 6 del; 10 mod 8257591: Remove suppression of record preview related warnings in java.lang Reviewed-by: chegar - PR: https://git.openjdk.java.net/jdk/pull/1591
Integrated: 8255845: Memory leak in imageFile.cpp
On Thu, 3 Dec 2020 14:57:16 GMT, Evan Whelan wrote: > This is a straightforward fix to resolve a potential memory leak in > imageFile.cpp. > > If `find_location` returns true, the `path` char array is never released. > > This has been fixed This pull request has now been integrated. Changeset: 66a2e709 Author:Evan Whelan Committer: Jim Laskey URL: https://git.openjdk.java.net/jdk/commit/66a2e709 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod 8255845: Memory leak in imageFile.cpp Reviewed-by: jlaskey, sundar - PR: https://git.openjdk.java.net/jdk/pull/1599
Re: RFR: 8255845: Memory leak in imageFile.cpp
On Thu, 3 Dec 2020 14:57:16 GMT, Evan Whelan wrote: > This is a straightforward fix to resolve a potential memory leak in > imageFile.cpp. > > If `find_location` returns true, the `path` char array is never released. > > This has been fixed Marked as reviewed by sundar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1599
Re: RFR: 8255845: Memory leak in imageFile.cpp
On Thu, 3 Dec 2020 14:57:16 GMT, Evan Whelan wrote: > This is a straightforward fix to resolve a potential memory leak in > imageFile.cpp. > > If `find_location` returns true, the `path` char array is never released. > > This has been fixed Marked as reviewed by jlaskey (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1599
RFR: 8255845: Memory leak in imageFile.cpp
This is a straightforward fix to resolve a potential memory leak in imageFile.cpp. If `find_location` returns true, the `path` char array is never released. This has been fixed - Commit messages: - 8255845: Memory leak in imageFile.cpp Changes: https://git.openjdk.java.net/jdk/pull/1599/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1599=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255845 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1599.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1599/head:pull/1599 PR: https://git.openjdk.java.net/jdk/pull/1599
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Fri, 20 Nov 2020 11:41:53 GMT, Severin Gehwolf wrote: >> Thanks @jerboaa >> >> I have a JBS number now: >> [8256515](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8256515). >> Since opening the PR I've learned that I've gone about the process >> backwards, and should have found a Sponsor before making the PR. As such, I >> haven't updated the commit message to avoid sending a premature RFR to the >> mailing list > > @MariusVolkhart Here is a PR for your branch so the test works with jtreg: > https://github.com/MariusVolkhart/jdk/pull/1 > > Fails before the JDK patch and passes after. > > Run it with: > $ rm -rf JTwork JTreport && jtreg > -jdk:./build/linux-x86_64-server-release/images/jdk -verbose:summary > test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java > Passed: javax/xml/jaxp/8256515/XmlInputFactoryTest.java > Test results: passed: 1 > Results are in *.jtr files: > > $ find JTwork/ -name *.jtr > JTwork/javax/xml/jaxp/8256515/XmlInputFactoryTest.jtr > > Or using the test framework of OpenJDK with: > > $ bash configure --with-jtreg=/path/to/jtreg [...] > $ make test TEST="jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java" > > HTH @jerboaa Thanks! That PR was immensely helpful, not just in helping get this change moving, but in helping me understand how to write the tests for next time! - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Thu, 19 Nov 2020 16:34:36 GMT, Marius Volkhart wrote: >> @MariusVolkhart I'll work with you getting the fix in and get the test >> running using jtreg. Give me a few moments to reproduce. > > Thanks @jerboaa > > I have a JBS number now: > [8256515](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8256515). > Since opening the PR I've learned that I've gone about the process backwards, > and should have found a Sponsor before making the PR. As such, I haven't > updated the commit message to avoid sending a premature RFR to the mailing > list Hi Marius, Saw the JBS issue, pls edit the title to sth. like the following: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event Thanks, Joe - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Wed, 4 Nov 2020 14:01:53 GMT, Marius Volkhart
wrote:
> The default implementation of javax.xml.stream.XMLEventReader produced a
> StartDocument event that always indicated that the "standalone" attribute was
> set.
>
> The root cause of this was that the
> com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the
> "standalone" attribute, rather than asking streamReader.standaloneSet()
> before setting the property of the StartDocumentEvent being created.
test/jdk/javax/xml/stream/XmlInputFactoryTest.java line 12:
> 10: import static org.testng.Assert.assertTrue;
> 11:
> 12: public class XmlInputFactoryTest {
This test is written correctly for regular TestNG, but I don't know if it's
"right" for jtreg. I'm happy to make any changes necessary.
-
PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Thu, 19 Nov 2020 16:34:36 GMT, Marius Volkhart wrote: >> @MariusVolkhart I'll work with you getting the fix in and get the test >> running using jtreg. Give me a few moments to reproduce. > > Thanks @jerboaa > > I have a JBS number now: > [8256515](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8256515). > Since opening the PR I've learned that I've gone about the process backwards, > and should have found a Sponsor before making the PR. As such, I haven't > updated the commit message to avoid sending a premature RFR to the mailing > list @MariusVolkhart I think doing a PR is fine before you find a sponsor provided you're fairly certain it's an actual bug. It'll require one to then send an email to the mailing lists asking for a sponsor. That email could potentially reference a PR. That should be fine. The sponsor (committer) will then help you get it integrated. It somewhat depends on the issue. No hard-and-fast rules that I'm aware of in that regard. @MariusVolkhart Here is a PR for your branch so the test works with jtreg: https://github.com/MariusVolkhart/jdk/pull/1 Fails before the JDK patch and passes after. Run it with: $ rm -rf JTwork JTreport && jtreg -jdk:./build/linux-x86_64-server-release/images/jdk -verbose:summary test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java Passed: javax/xml/jaxp/8256515/XmlInputFactoryTest.java Test results: passed: 1 Results are in *.jtr files: $ find JTwork/ -name *.jtr JTwork/javax/xml/jaxp/8256515/XmlInputFactoryTest.jtr Or using the test framework of OpenJDK with: $ bash configure --with-jtreg=/path/to/jtreg [...] $ make test TEST="jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java" HTH - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Wed, 18 Nov 2020 09:42:49 GMT, Severin Gehwolf wrote: >> The default implementation of javax.xml.stream.XMLEventReader produced a >> StartDocument event that always indicated that the "standalone" attribute >> was set. >> >> The root cause of this was that the >> com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the >> "standalone" attribute, rather than asking streamReader.standaloneSet() >> before setting the property of the StartDocumentEvent being created. > > @MariusVolkhart I'll work with you getting the fix in and get the test > running using jtreg. Give me a few moments to reproduce. Thanks @jerboaa I have a JBS number now: [8256515](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8256515). Since opening the PR I've learned that I've gone about the process backwards, and should have found a Sponsor before making the PR. As such, I haven't updated the commit message to avoid sending a premature RFR to the mailing list - PR: https://git.openjdk.java.net/jdk/pull/1056
RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
The default implementation of javax.xml.stream.XMLEventReader produced a StartDocument event that always indicated that the "standalone" attribute was set. The root cause of this was that the com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the "standalone" attribute, rather than asking streamReader.standaloneSet() before setting the property of the StartDocumentEvent being created. - Commit messages: - Merge pull request #1 from jerboaa/pull-1056-amend - Adjust test so it works with jtreg - Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT event - Add test for XmlInputFactory Changes: https://git.openjdk.java.net/jdk/pull/1056/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1056=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256515 Stats: 47 lines in 2 files changed: 46 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1056/head:pull/1056 PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Wed, 4 Nov 2020 14:01:53 GMT, Marius Volkhart wrote: > The default implementation of javax.xml.stream.XMLEventReader produced a > StartDocument event that always indicated that the "standalone" attribute was > set. > > The root cause of this was that the > com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the > "standalone" attribute, rather than asking streamReader.standaloneSet() > before setting the property of the StartDocumentEvent being created. @MariusVolkhart I'll work with you getting the fix in and get the test running using jtreg. Give me a few moments to reproduce. - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Thu, 3 Dec 2020 14:08:48 GMT, Jan Lahoda wrote: >>> I suggest `Class::getPermittedSubclasses` to return a `non-null` array if >>> this `Class` is sealed, i.e. this class is derived from a `class` file with >>> the presence of `PermittedSubclasses` attribute regardless of its content >>> (the attribute could be empty or contains zero or more entries which is a >>> properly loaded permitted subtype. >>> >>> If this `Class` is not sealed, `Class::getPermittedSubclasses` returns null >>> (see `Class::getRecordComponents` and some other reflection APIs as >>> precedence). >> >> Agree, that seems reasonable. Often, methods in `Class` with an array return >> type default to an empty array, but `getRecordComponents` is a good example >> of returning `null` when an empty array is meaningful. > > I've changed Class.getPermittedSubclasses to return null for classes that are > not sealed here: > https://github.com/openjdk/jdk/pull/1483/commits/7056143822ff62dfbb1d294c67352ed3892317c2 > with follow-up changes to tests here: > https://github.com/openjdk/jdk/pull/1483/commits/537e267fb6802ab5cf38bf26978039383cc6309a > > How does this look? After a discussion with Harold, I've reverted the patch where Class.getPermittedSubclasses returns null. Harold will do that separatelly under JDK-8256867, unless there are objections. The changes that were reverted are still available here: https://openjdk.github.io/cr/?repo=jdk=1483=02-03 Please let me know if there are any issues. - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8166026: Refactor java/lang shell tests to java
On Wed, 2 Dec 2020 20:45:11 GMT, Ivan Šipka wrote: > Refactor > `test/jdk/java/lang/SecurityManager/modules/CustomSecurityManager.sh` as java > test. Marked as reviewed by mullan (Reviewer). test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java line 2: > 1: /* > 2: * Copyright (c) 2015, 2020 Oracle and/or its affiliates. All rights > reserved. You are missing a comma after "2020". - PR: https://git.openjdk.java.net/jdk/pull/1579
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Wed, 2 Dec 2020 19:31:59 GMT, Dan Smith wrote: >>> Additional changes may be needed to Class.permittedSubclasses() and/or >>> Class.isSealed() as part of fixing bug JDK-8256867. The JVM is being >>> changed to treat classes with empty PermittedSubclasses attributes as >>> sealed classes that cannot be extended (or implemented). >>> >>> Current thinking is that Class.permittedSubclasses() will return an empty >>> array for both non-sealed classes and for sealed classes with empty >>> PermittedSubclasses attributes. And, Class.isSealed() will return False in >>> the former case and True in the latter. This will require changing the >>> implementation of Class.isSealed() to call the JVM directly instead of >>> calling Class.permittedSubclasses(). >>> >>> Does this seem like a reasonable way to handle this corner case? >> >> I suggest `Class::getPermittedSubclasses` to return a `non-null` array if >> this `Class` is sealed, i.e. this class is derived from a `class` file with >> the presence of `PermittedSubclasses` attribute regardless of its content >> (the attribute could be empty or contains zero or more entries which is a >> properly loaded permitted subtype. >> >> If this `Class` is not sealed, `Class::getPermittedSubclasses` returns null >> (see `Class::getRecordComponents` and some other reflection APIs as >> precedence). > >> I suggest `Class::getPermittedSubclasses` to return a `non-null` array if >> this `Class` is sealed, i.e. this class is derived from a `class` file with >> the presence of `PermittedSubclasses` attribute regardless of its content >> (the attribute could be empty or contains zero or more entries which is a >> properly loaded permitted subtype. >> >> If this `Class` is not sealed, `Class::getPermittedSubclasses` returns null >> (see `Class::getRecordComponents` and some other reflection APIs as >> precedence). > > Agree, that seems reasonable. Often, methods in `Class` with an array return > type default to an empty array, but `getRecordComponents` is a good example > of returning `null` when an empty array is meaningful. I've changed Class.getPermittedSubclasses to return null for classes that are not sealed here: https://github.com/openjdk/jdk/pull/1483/commits/7056143822ff62dfbb1d294c67352ed3892317c2 with follow-up changes to tests here: https://github.com/openjdk/jdk/pull/1483/commits/537e267fb6802ab5cf38bf26978039383cc6309a How does this look? - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v4]
> This pull request replaces https://github.com/openjdk/jdk/pull/1227. > > From the original PR: > >> Please review the code for the second iteration of sealed classes. In this >> iteration we are: >> >> * Enhancing narrowing reference conversion to allow for stricter >> checking of cast conversions with respect to sealed type hierarchies >> >> * Also local classes are not considered when determining implicitly >> declared permitted direct subclasses of a sealed class or sealed interface >> >> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >> still in the same method, the return type has been changed to Class[] >> instead of the previous ClassDesc[] >> >> * adding code to make sure that annotations can't be sealed >> >> * improving some tests >> >> >> TIA >> >> Related specs: >> [Sealed Classes >> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >> [Sealed Classes >> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >> [Additional: Contextual >> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) > > This PR strives to reflect the review comments from 1227: > * adjustments to javadoc of j.l.Class methods > * package access checks in Class.getPermittedSubclasses() > * fixed to the narrowing conversion/castability as pointed out by Maurizio Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Fixing tests. - getPermittedSubclasses to return null for non-sealed classes, as requested. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1483/files - new: https://git.openjdk.java.net/jdk/pull/1483/files/ff1abf06..537e267f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1483=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1483=02-03 Stats: 117 lines in 6 files changed: 94 ins; 5 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1483.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1483/head:pull/1483 PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 09:58:16 GMT, Alan Bateman wrote:
>> The attribute_length of known Module attributes in the module-info.class
>> is currently ignored. It should be checked and the class rejected if the
>> attribute length doesn't exactly match the length of the info in the
>> attribute.
>>
>> There are several ways to fix this. I initially limited the reading of the
>> attribute_info to the attribute length but this resulted in confusing
>> exception messages as the attribute appears truncated. The exception
>> messages are clearer when it checks that the attribute length corresponds to
>> the number of bytes read.
>
> Alan Bateman has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 11 additional
> commits since the last revision:
>
> - Restructure check to make it more obvious that it doesn't overflow
> - Merge
> - Merge
> - Merge
> - Trailing whitespace
> - Expand test to Module attribute
> - Merge
> - Test cleanup
> - Add test
> - Merge
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/914dd7e9...f15dbb1b
src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 1203:
> 1201: @Override
> 1202: public String readUTF() throws IOException {
> 1203: return DataInputStream.readUTF(this);
If i understand correctly the code, I believe readUTF should change a boolean
field named `countCanNotBeTrackedAnymore` from false to true, and in the method
`count()`, `countCanNotBeTrackedAnymore` has to be checked and throws an ISE
before returning `count`
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 09:58:16 GMT, Alan Bateman wrote: >> The attribute_length of known Module attributes in the module-info.class >> is currently ignored. It should be checked and the class rejected if the >> attribute length doesn't exactly match the length of the info in the >> attribute. >> >> There are several ways to fix this. I initially limited the reading of the >> attribute_info to the attribute length but this resulted in confusing >> exception messages as the attribute appears truncated. The exception >> messages are clearer when it checks that the attribute length corresponds to >> the number of bytes read. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - Restructure check to make it more obvious that it doesn't overflow > - Merge > - Merge > - Merge > - Trailing whitespace > - Expand test to Module attribute > - Merge > - Test cleanup > - Add test > - Merge > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/575caaeb...f15dbb1b Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8257591: Remove suppression of record preview related warnings in java.lang
On Thu, 3 Dec 2020 10:39:05 GMT, Julia Boes wrote: > Records exit preview in JDK 16. This change removes preview related > suppression warnings in some source files and removes the '--enable-preview' > option for compilation and execution of some tests that use record classes. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1591
RFR: 8257591: Remove suppression of record preview related warnings in java.lang
Records exit preview in JDK 16. This change removes preview related suppression warnings in some source files and removes the '--enable-preview' option for compilation and execution of some tests that use record classes. - Commit messages: - remove --enable-preview in tests - remove record related warning suppression Changes: https://git.openjdk.java.net/jdk/pull/1591/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1591=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257591 Stats: 16 lines in 6 files changed: 0 ins; 6 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/1591.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1591/head:pull/1591 PR: https://git.openjdk.java.net/jdk/pull/1591
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 09:58:16 GMT, Alan Bateman wrote:
>> The attribute_length of known Module attributes in the module-info.class
>> is currently ignored. It should be checked and the class rejected if the
>> attribute length doesn't exactly match the length of the info in the
>> attribute.
>>
>> There are several ways to fix this. I initially limited the reading of the
>> attribute_info to the attribute length but this resulted in confusing
>> exception messages as the attribute appears truncated. The exception
>> messages are clearer when it checks that the attribute length corresponds to
>> the number of bytes read.
>
> Alan Bateman has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 11 additional
> commits since the last revision:
>
> - Restructure check to make it more obvious that it doesn't overflow
> - Merge
> - Merge
> - Merge
> - Trailing whitespace
> - Expand test to Module attribute
> - Merge
> - Test cleanup
> - Add test
> - Merge
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/03216be8...f15dbb1b
Marked as reviewed by dfuchs (Reviewer).
src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 288:
> 286:
> 287: long newPosition = in.count();
> 288: if ((newPosition - initialPosition) != length) {
LGTM! Thanks for making the change.
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
> The attribute_length of known Module attributes in the module-info.class > is currently ignored. It should be checked and the class rejected if the > attribute length doesn't exactly match the length of the info in the > attribute. > > There are several ways to fix this. I initially limited the reading of the > attribute_info to the attribute length but this resulted in confusing > exception messages as the attribute appears truncated. The exception messages > are clearer when it checks that the attribute length corresponds to the > number of bytes read. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Restructure check to make it more obvious that it doesn't overflow - Merge - Merge - Merge - Trailing whitespace - Expand test to Module attribute - Merge - Test cleanup - Add test - Merge - ... and 1 more: https://git.openjdk.java.net/jdk/compare/1586643c...f15dbb1b - Changes: - all: https://git.openjdk.java.net/jdk/pull/1407/files - new: https://git.openjdk.java.net/jdk/pull/1407/files/600db8cf..f15dbb1b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1407=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1407=00-01 Stats: 4917 lines in 168 files changed: 4255 ins; 240 del; 422 mod Patch: https://git.openjdk.java.net/jdk/pull/1407.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1407/head:pull/1407 PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v3]
On Tue, 1 Dec 2020 12:06:25 GMT, Jan Lahoda wrote: >> test/langtools/tools/javac/platform/createsymbols/CreateSymbolsTestImpl.java >> line 513: >> >>> 511: public java.util.List >>> l(); >>> 512:} >>> 513:""", >> >> I don't understand why the lines with `\n` in a text block > > There is a combination of factors here: > -jcheck (AFAIK) does not allow trailing whitespaces, even not on otherwise > empty lines inside textblocks > -textblocks only remove indentation that is common on all lines. > > So, without having '\n', we would have to strip all the whitespace on the > empty lines (to pass jcheck), which would mean the text block's content would > no longer match the output. There are a few ways to solve this (almost surely > an incomplete list): > -do some trick to have the common indent, but no trailing whitespace. '\n' is > one of them. > -not indent the text block > -do some post-processing on the text block's value or the actual test output, > to make them match > -not use textblocks I stand corrected here - blank lines do not count when the common indent is computed. Removed here: https://github.com/openjdk/jdk/pull/1480/commits/3aaaf28c23ddda71c77ca9923e02e5f3502cde3b - PR: https://git.openjdk.java.net/jdk/pull/1480
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v5]
> Adding support for record classes in the historical data for ct.sym. This > includes a few changes not strictly needed for the change: > -updating and moving tests into test/langtools, so that it is easier to run > them. > -fixing Record attribute reading in javac's ClassReader (used for tests, but > seems like the proper thing to do anyway). > -fixing the -Xprint annotation processor to print record component > annotations. > > Changes to jdk.jdeps' classfile library are needed so that the ct.sym > creation works. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Blank lines do not count for the text block indentation, removing them. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1480/files - new: https://git.openjdk.java.net/jdk/pull/1480/files/9ab7153a..3aaaf28c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1480=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1480=03-04 Stats: 15 lines in 1 file changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/1480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1480/head:pull/1480 PR: https://git.openjdk.java.net/jdk/pull/1480
