Re: RFR: 8203035: Implement equals() and hashCode() for Throwable
On Sat, 10 Dec 2022 18:11:30 GMT, Victor Toni wrote: > Being able to compare instances of Throwable allows simple detection of > exceptions raised by the same circumstances. Comparison allows for reduction > of excessive logging e.g. in hotspots without requiring custom code to > compare Throwables. @ViToni I don't know if you have JBS access but can you read through the comments JDK-8203035 to see the concerns with this proposal? - PR: https://git.openjdk.org/jdk/pull/11624
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 18:34:57 GMT, Per Minborg wrote: > On the note of `CHM::isEmpty`: It would be better to rewrite this method as a > short-circuitable reduction of the many CounterCells' values. As soon as at > least one of them are >0 then the map is not empty. In contrast, today we sum > all of the values, in many cases, unnecessary. Well, turns out this is harder than said and the original solution is not that bad after all. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes
On Mon, 9 Jan 2023 20:55:14 GMT, Andrey Turbanov wrote: > `jdk.internal.jrtfs.JrtFileSystem#getFileAttributes` never return `null` > > https://github.com/openjdk/jdk/blob/679e485838881c1364845072af305fb60d95e60a/src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java#L206-L213 > So, no need to check for `null` its result. > Seems it was copied from ZipPath (name of variable suggests that) - > https://github.com/openjdk/jdk/blob/master/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java#L769. > But for ZipFileSystem `null` is expected. This looks okay as the 2-arg getFileAttrbutes will throw if the entry doesn't exist in the file system. I assume you'll update the end copyright year to 2023 before you integrate this. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/11911
Re: RFR: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes [v2]
> `jdk.internal.jrtfs.JrtFileSystem#getFileAttributes` never return `null` > > https://github.com/openjdk/jdk/blob/679e485838881c1364845072af305fb60d95e60a/src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java#L206-L213 > So, no need to check for `null` its result. > Seems it was copied from ZipPath (name of variable suggests that) - > https://github.com/openjdk/jdk/blob/master/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java#L769. > But for ZipFileSystem `null` is expected. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes update copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/11911/files - new: https://git.openjdk.org/jdk/pull/11911/files/6237726d..74068b94 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11911&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11911&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11911.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11911/head:pull/11911 PR: https://git.openjdk.org/jdk/pull/11911
ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
Hello.
I've noticed that ZipFileStore#supportsFileAttributeView(String)
doesn't throw NullPointerException when 'null' is passed as an
argument.
public boolean supportsFileAttributeView(String name) {
return "basic".equals(name) || "zip".equals(name) ||
(("owner".equals(name) || "posix".equals(name)) && zfs.supportPosix);
}
Method body was changed to not throw NPE in JDK-8213031.
I wonder if it was an intentional change or not?
My reproducer shows difference between ZipFileStore and WindowsFileStore
public static void main(String[] args) throws IOException {
checkFsForNull(FileSystems.newFileSystem(Path.of("D:\\config.zip")));
checkFsForNull(FileSystems.getDefault());
}
private static void checkFsForNull(FileSystem fs) {
fs.getFileStores().iterator().next().supportsFileAttributeView((String)null);
}
Andrey Turbanov
Re: jpackageapplauncher linker arguments for osx
Hi Alexey, great! I've created the PR: https://github.com/openjdk/jdk/pull/11922 Best regards, David Schumann Am Mo., 9. Jan. 2023 um 23:57 Uhr schrieb Alexey Semenyuk < alexey.semen...@oracle.com>: > Hi David, > > The request to adjust osx linker command lines looks reasonable. Please > go ahead with a pull request. > > - Alexey > > On 1/9/2023 1:21 AM, David Holmes wrote: > > On 8/01/2023 8:39 pm, David Schumann wrote: > >> Hello, > >> > >> I'm not 100% sure if this list is the correct one for this topic, > >> feel free to redirect me. > > > > core-libs-dev - cc'd > > > > Cheers, > > David > > > >> Currently the jpackageapplauncher binary gets linked without the > >> "-headerpad_max_install_names" argument on osx. This prevents a user > >> from using the install_name_tool with the launcher binary. > >> This means that it's currently not possible to include dylibs in the > >> application bundle. > >> > >> To provide a bit more context: > >> I'm using jpackage to build an app bundle. The java application > >> itself uses external native libraries, which internally use dlopen to > >> reference other libs. Normaly one would include the other libs in the > >> app bundle, and use the install_name_tool to tell the dynamic linker > >> which paths to load the libraries from. However since the launcher > >> binary doesn't get linked with the correct arguments, this isn't > >> possible. > >> > >> I wanted to discuss this change, and it it makes sense for me to open > >> a pull request for it. > >> The change would be in > >> > https://github.com/openjdk/jdk/blob/master/make/modules/jdk.jpackage/Lib.gmk#L76 > >> < > https://github.com/openjdk/jdk/blob/master/make/modules/jdk.jpackage/Lib.gmk#L76> > > >> > >> > >> Best Regards, > >> David Schumann > >> > >
Re: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
On 10/01/2023 09:35, Andrey Turbanov wrote:
Hello.
I've noticed that ZipFileStore#supportsFileAttributeView(String)
doesn't throw NullPointerException when 'null' is passed as an
argument.
public boolean supportsFileAttributeView(String name) {
return "basic".equals(name) || "zip".equals(name) ||
(("owner".equals(name) || "posix".equals(name)) &&
zfs.supportPosix);
}
Method body was changed to not throw NPE in JDK-8213031.
I wonder if it was an intentional change or not?
The review of that feature went through many iterations over 8 months
and this was missed, so maybe a short coming in the zipfs tests. Can you
create a bug for this?
-Alan.
RFR: 8299852: Modernize ConcurrentHashMap
`java.util.concurrent.ConcurrentHashMap` is relatively old and has not been updated to reflect the current state of Java and can be modernized: * Add `@Serial` annotations * Seal classes and restrict subclassing for internal classes * Use pattern matching for instance * Remove redundant modifiers (such as some final declarations) * Use Objects.requireNonNull for invariant checking * Use diamond operators instead of explicit types * Simplify expressions using Math::max - Commit messages: - Untrack file - Modernize ConcurrentHashMap Changes: https://git.openjdk.org/jdk/pull/11924/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11924&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299852 Stats: 385 lines in 1 file changed: 14 ins; 22 del; 349 mod Patch: https://git.openjdk.org/jdk/pull/11924.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11924/head:pull/11924 PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes [v2]
On Tue, 10 Jan 2023 09:14:11 GMT, Andrey Turbanov wrote: >> `jdk.internal.jrtfs.JrtFileSystem#getFileAttributes` never return `null` >> >> https://github.com/openjdk/jdk/blob/679e485838881c1364845072af305fb60d95e60a/src/java.base/share/classes/jdk/internal/jrtfs/JrtFileSystem.java#L206-L213 >> So, no need to check for `null` its result. >> Seems it was copied from ZipPath (name of variable suggests that) - >> https://github.com/openjdk/jdk/blob/master/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java#L769. >> But for ZipFileSystem `null` is expected. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes > > update copyright year Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11911
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote: > `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been > updated to reflect the current state of Java and can be modernized: > > * Add `@Serial` annotations > * Seal classes and restrict subclassing for internal classes > * Use pattern matching for instance > * Remove redundant modifiers (such as some final declarations) > * Use Objects.requireNonNull for invariant checking > * Use diamond operators instead of explicit types > * Simplify expressions using Math::max I'm sure others will note or have already noted elsewhere that we don't do direct PRs against JSR 166. - PR: https://git.openjdk.org/jdk/pull/11924
Re: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
Filled https://bugs.openjdk.org/browse/JDK-8299864
Andrey Turbanov
вт, 10 янв. 2023 г. в 13:24, Alan Bateman :
>
>
>
> On 10/01/2023 09:35, Andrey Turbanov wrote:
> > Hello.
> > I've noticed that ZipFileStore#supportsFileAttributeView(String)
> > doesn't throw NullPointerException when 'null' is passed as an
> > argument.
> >
> > public boolean supportsFileAttributeView(String name) {
> > return "basic".equals(name) || "zip".equals(name) ||
> > (("owner".equals(name) || "posix".equals(name)) &&
> > zfs.supportPosix);
> > }
> >
> > Method body was changed to not throw NPE in JDK-8213031.
> > I wonder if it was an intentional change or not?
> The review of that feature went through many iterations over 8 months
> and this was missed, so maybe a short coming in the zipfs tests. Can you
> create a bug for this?
>
> -Alan.
Re: RFR: 8299513: Cleanup java.io [v5]
> Code in java.io contains many legacy constructs and semantics not recommended > including: > > * C-style array declaration > * Unnecessary visibility > * Redundant keywords in interfaces (e.g. public, static) > * Non-standard naming for constants > * Javadoc typos > * Missing final declaration > > These should be fixed as a sanity effort. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Revert removal of a final keyword - Changes: - all: https://git.openjdk.org/jdk/pull/11848/files - new: https://git.openjdk.org/jdk/pull/11848/files/a0e80355..78803a1a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11848&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11848&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11848.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11848/head:pull/11848 PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 12:04:10 GMT, Pavel Rappo wrote: > I'm sure others will note or have already noted elsewhere that we don't do > direct PRs against JSR 166. Yes. I will change the PR to a draft as we explore ways to integrate the spirit of the proposed changes. - PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8293841: RISC-V: Implementation of Foreign Function & Memory API (Preview) [v4]
> Add experimental Foreign Function & Memory API support for RISC-V. > > For details of the FFM API RISC-V port please refer to [JBS > issue](https://bugs.openjdk.org/browse/JDK-8293841) > > Testing: > > - [x] jdk_foreign with release/fastdebug build > - [x] run TestMatrix.java manually with release/fastdebug build Feilong Jiang has updated the pull request incrementally with one additional commit since the last revision: fix callArranger - Changes: - all: https://git.openjdk.org/jdk/pull/11004/files - new: https://git.openjdk.org/jdk/pull/11004/files/289b8697..fd312661 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11004&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11004&range=02-03 Stats: 6 lines in 2 files changed: 0 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/11004.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11004/head:pull/11004 PR: https://git.openjdk.org/jdk/pull/11004
[jdk20] RFR: 8299862: OfAddress setter should disallow heap segments
When unifying memory address with memory segments, we missed the case where a heap memory segment is passed as a value to a var handle address setters. The solution is to reuse the same check we use when validating segment downcall parameters also for segment memory writes. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk20/pull/92/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=92&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299862 Stats: 27 lines in 5 files changed: 23 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk20/pull/92.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/92/head:pull/92 PR: https://git.openjdk.org/jdk20/pull/92
Re: [jdk20] RFR: 8299862: OfAddress setter should disallow heap segments
On Tue, 10 Jan 2023 14:04:38 GMT, Maurizio Cimadamore wrote: > When unifying memory address with memory segments, we missed the case where a > heap memory segment is passed as a value to a var handle address setters. > > The solution is to reuse the same check we use when validating segment > downcall parameters also for segment memory writes. Thanks for fixing. - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.org/jdk20/pull/92
Re: RFR: 8299513: Cleanup java.io [v5]
On Tue, 10 Jan 2023 13:34:49 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of a final keyword src/java.base/share/classes/java/io/ObjectStreamConstants.java line 38: > 36: * Magic number that is written to the stream header. > 37: */ > 38: short STREAM_MAGIC = (short)0xaced; I'd prefer to retain the `static`, it is easier to read and not have to remember that this declaration is in an interface. - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8293841: RISC-V: Implementation of Foreign Function & Memory API (Preview) [v5]
> Add experimental Foreign Function & Memory API support for RISC-V. > > For details of the FFM API RISC-V port please refer to [JBS > issue](https://bugs.openjdk.org/browse/JDK-8293841) > > Testing: > > - [x] jdk_foreign with release/fastdebug build > - [x] run TestMatrix.java manually with release/fastdebug build Feilong Jiang has updated the pull request incrementally with one additional commit since the last revision: fix callArranger - Changes: - all: https://git.openjdk.org/jdk/pull/11004/files - new: https://git.openjdk.org/jdk/pull/11004/files/fd312661..fee96ae8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11004&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11004&range=03-04 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11004.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11004/head:pull/11004 PR: https://git.openjdk.org/jdk/pull/11004
RFR: 8299864: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
This PR proposes to add null-checking for some parameter arguments in `ZipFileStore`. - Commit messages: - Check null invariants Changes: https://git.openjdk.org/jdk/pull/11926/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11926&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299864 Stats: 49 lines in 2 files changed: 26 ins; 1 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/11926.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11926/head:pull/11926 PR: https://git.openjdk.org/jdk/pull/11926
Re: RFR: 8299513: Cleanup java.io [v5]
On Tue, 10 Jan 2023 14:59:35 GMT, Roger Riggs wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert removal of a final keyword > > src/java.base/share/classes/java/io/ObjectStreamConstants.java line 38: > >> 36: * Magic number that is written to the stream header. >> 37: */ >> 38: short STREAM_MAGIC = (short)0xaced; > > I'd prefer to retain the `static`, it is easier to read and not have to > remember that this declaration is in an interface. I think the class should not be an interface in the first place but this is hard to change now. - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299513: Cleanup java.io [v6]
> Code in java.io contains many legacy constructs and semantics not recommended > including: > > * C-style array declaration > * Unnecessary visibility > * Redundant keywords in interfaces (e.g. public, static) > * Non-standard naming for constants > * Javadoc typos > * Missing final declaration > > These should be fixed as a sanity effort. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Revert removal of static modifier in OSC - Changes: - all: https://git.openjdk.org/jdk/pull/11848/files - new: https://git.openjdk.org/jdk/pull/11848/files/78803a1a..f231c16b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11848&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11848&range=04-05 Stats: 32 lines in 1 file changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/11848.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11848/head:pull/11848 PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299836: Make `user.timezone` system property searchable [v2]
On Tue, 10 Jan 2023 00:46:35 GMT, Justin Lu wrote: >> The system property _user.timezone_ is specified in the >> _TimeZone.getDefault()_ and _TimeZone.setDefault()_ methods. The javadoc >> search box should be able to match on the system property name. >> >> This change replaces the **@code** tag with the **@systemProperty** tag for >> instances of _user.timezone_ when appropriate. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Toggle first instance as a system property, revert rest LGTM. Thanks for the fix. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk/pull/11915
ZipFile.isSignatureRelated returns true for files in META-INF subdirectories
Hi, ZipFile.isSignatureRelated currently returns true for paths such as the following: META-INF/libraries/org.bouncycastle:bcprov-jdk15on:jar-1.70/META-INF/BC2048KE.DSA While this path does start with "META-INF/" and ends with ".DSA", the file does not live in the META-INF/ directory _directly_, but rather several directories below. This causes such .DSA files to be incorrectly (?) included in the verification of META-INF/MANIFEST.MF in JarFile.initializeVerifier, which then fails with: Exception in thread "main" java.lang.SecurityException: Invalid signature > file digest for Manifest main attributes > at > java.base/sun.security.util.SignatureFileVerifier.processImpl(SignatureFileVerifier.java:340) > at > java.base/sun.security.util.SignatureFileVerifier.process(SignatureFileVerifier.java:282) > at java.base/java.util.jar.JarVerifier.processEntry(JarVerifier.java:327) > at java.base/java.util.jar.JarVerifier.update(JarVerifier.java:239) > at java.base/java.util.jar.JarFile.initializeVerifier(JarFile.java:760) > at java.base/java.util.jar.JarFile.ensureInitialization(JarFile.java:1058) > at > java.base/java.util.jar.JavaUtilJarAccessImpl.ensureInitialization(JavaUtilJarAccessImpl.java:72) > at > java.base/jdk.internal.loader.URLClassPath$JarLoader$2.getManifest(URLClassPath.java:883) > at > java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:848) > at > java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760) > at > java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681) > at > java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639) > at > java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) > at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) A few questions: 1: Where Is the exact location of signature related files specified? 2: Is the current behaviour indeed incorrect? 3: Should ZipFile.isSignatureRelated be updated such that it only matches signature related files which reside exactly in "META-INF/" ? The context for this is that I'm making a fat jar Maven plugin which embeds dependency jars by "unpacking" them into subdirectories of META-INF/libraries/. Cheers, Eirik.
Integrated: 8299183: Invokers.checkExactType passes parameters to create WMTE in opposite order
On Thu, 5 Jan 2023 20:45:20 GMT, Mandy Chung wrote: > Trivial fix. Fix `Invokers.checkExactType` to call > `newWrongMethodTypeException(actual, expected)` with parameters in right > order. This pull request has now been integrated. Changeset: a86b6f6f Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/a86b6f6fde11a1cb27f926c43d53585049fed5e4 Stats: 103 lines in 4 files changed: 85 ins; 1 del; 17 mod 8299183: Invokers.checkExactType passes parameters to create WMTE in opposite order Reviewed-by: iris, jpai - PR: https://git.openjdk.org/jdk/pull/11870
RFR: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
Fixing the subject method to recover gracefully on a failed `ZoneRulesProvider` registration. - Commit messages: - Added a test - 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure Changes: https://git.openjdk.org/jdk/pull/11928/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11928&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299571 Stats: 99 lines in 2 files changed: 98 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11928.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11928/head:pull/11928 PR: https://git.openjdk.org/jdk/pull/11928
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC The `static` declarations should be restored in ObjectStreamConstants. - Changes requested by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299513: Clean up java.io [v5]
On Tue, 10 Jan 2023 15:52:51 GMT, Per Minborg wrote: >> src/java.base/share/classes/java/io/ObjectStreamConstants.java line 38: >> >>> 36: * Magic number that is written to the stream header. >>> 37: */ >>> 38: short STREAM_MAGIC = (short)0xaced; >> >> I'd prefer to retain the `static`, it is easier to read and not have to >> remember that this declaration is in an interface. > > I think the class should not be an interface in the first place but this is > hard to change now. Just because the static is not required does NOT mean it should be omitted!. - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299864: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
On Tue, 10 Jan 2023 15:26:05 GMT, Per Minborg wrote:
> This PR proposes to add null-checking for some parameter arguments in
> `ZipFileStore`.
Thanks for taking this on Per.
I think we also need to add a test for getAttribute() and
getFileStoreAttributeView() as I do not see it being tested in
test/jdk/jdk/nio/zipfs/ZipFSTester.java or test/jdk/jdk/nio/zipfs/Basic.java
A few more minor comments below:
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileStore.java line 43:
> 41: * @author Xueming Shen, Rajendra Gutupalli, Jaya Hangal
> 42: */
> 43: final class ZipFileStore extends FileStore {
This should be OK but might suggest adding a release note
test/jdk/jdk/nio/zipfs/ZipFSTester.java line 1086:
> 1084: }
> 1085:
> 1086: static void test8299864(FileSystem fs) {
Please add a comment explaining the test and change the method name as would
prefer to have names that are a bit more descriptive than a bug number.
I realize some of the existing methods follow the same naming as you are
proposing, but we are trying to avoid this and make the tests more descriptive
going forward
-
PR: https://git.openjdk.org/jdk/pull/11926
Re: RFR: 8299836: Make `user.timezone` system property searchable [v2]
On Tue, 10 Jan 2023 01:50:45 GMT, Jaikiran Pai wrote: >> Like Naoto notes, my understanding of it is that the `@systemProperty` >> should appear only once at the place where the semantics of that system >> property is being defined. This mail, which was sent when this tag was >> introduced, has more details >> https://mail.openjdk.org/pipermail/core-libs-dev/2018-November/056653.html. >> >> I think using it here is fine, but you may want to check how it "reads" on >> the generated javadoc index page for system properties. > >> but you may want to check how it "reads" on the generated javadoc index page >> for system properties. > > For clarity, I meant the page where you will see all the system properties - > there's a "Index" on top of the javadoc API page > https://docs.oracle.com/en/java/javase/19/docs/api/index.html which leads to > a page which has a link to the "System Properties" page, which should lead to > https://docs.oracle.com/en/java/javase/19/docs/api/system-properties.html Hi Jai, I read the article and have a better understanding of when _@systemProperty_ should be used and the supporting features. I wasn't aware such a [page](https://docs.oracle.com/en/java/javase/19/docs/api/system-properties.html) existed, I checked that _user.timezone_ was displayed and linked to _java.util.TimeZone.getDefault()_ properly. Thanks for the help! - PR: https://git.openjdk.org/jdk/pull/11915
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
> The javadocs of the following methods used deprecated constructors of the > primitive wrapper classes: > > java.lang.ArrayStoreException > java.lang.ClassCastException > java.lang.Double.compare(double, double) > java.lang.Float.compare(float, float) > java.lang.Integer.getInteger(String, int) > java.lang.Integer.valueOf(String) > java.lang.Integer.valueOf(String, int) > java.lang.Long.getLong(String, long) > java.lang.Long.valueOf(String) > java.lang.Long.valueOf(String, int) > java.lang.Short.valueOf(String) > java.lang.Short.valueOf(String, int) > > This change replaces the constructors with .valueOf() methods except > java.lang.ClassCastException which was already fixed in JDK-8289730 Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Include Byte in changes - Changes: - all: https://git.openjdk.org/jdk/pull/11912/files - new: https://git.openjdk.org/jdk/pull/11912/files/fc79e191..eb88d193 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11912&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11912&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/11912.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11912/head:pull/11912 PR: https://git.openjdk.org/jdk/pull/11912
Integrated: 8298735: Some tools/jpackage/windows/* tests fails with jtreg test timeout
On Mon, 9 Jan 2023 22:53:18 GMT, Alexey Semenyuk wrote: > Increase failed test timeouts. > Simple tests got an x1.5 increase and parametrized tests got an x2 increase This pull request has now been integrated. Changeset: 3c99e786 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/3c99e786ab4f89448f8d2a6eaf532255a8a560bf Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod 8298735: Some tools/jpackage/windows/* tests fails with jtreg test timeout Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/11914
Integrated: 8299278: tools/jpackage/share/AddLauncherTest.java#id1 failed AddLauncherTest.bug8230933
On Thu, 5 Jan 2023 20:10:45 GMT, Alexey Semenyuk wrote: > 8299278: tools/jpackage/share/AddLauncherTest.java#id1 failed > AddLauncherTest.bug8230933 This pull request has now been integrated. Changeset: c595f965 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/c595f965abcf0ea80ace87b8f2180feebbb8984e Stats: 47 lines in 2 files changed: 41 ins; 0 del; 6 mod 8299278: tools/jpackage/share/AddLauncherTest.java#id1 failed AddLauncherTest.bug8230933 Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/11868
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote: >> The javadocs of the following methods used deprecated constructors of the >> primitive wrapper classes: >> >> java.lang.ArrayStoreException >> java.lang.ClassCastException >> java.lang.Double.compare(double, double) >> java.lang.Float.compare(float, float) >> java.lang.Integer.getInteger(String, int) >> java.lang.Integer.valueOf(String) >> java.lang.Integer.valueOf(String, int) >> java.lang.Long.getLong(String, long) >> java.lang.Long.valueOf(String) >> java.lang.Long.valueOf(String, int) >> java.lang.Short.valueOf(String) >> java.lang.Short.valueOf(String, int) >> >> This change replaces the constructors with .valueOf() methods except >> java.lang.ClassCastException which was already fixed in JDK-8289730 > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Include Byte in changes Given that type conversions create the primitive wrapper instances when they are needed, I think the examples should be reconsidered. They needlessly suggest that the wrapper instances need to be explicitly created. - Changes requested by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC Please do not type the "/integrate" when none of the reviewer approved the latest meaningful version - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:07:54 GMT, Roger Riggs wrote: > Given that type conversions create the primitive wrapper instances when they > are needed, I think the examples should be reconsidered. They needlessly > suggest that the wrapper instances need to be explicitly created. Hi Roger, thanks for the comment, When I was deciding between autoboxing and using the valueOf() methods, I personally saw the latter as providing more clarity from a documentation standpoint but I understand your concerns. Curious what others think on the decision of using one vs the other. - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Sat, 7 Jan 2023 21:08:07 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix incorrect @bug numbers in unit tests.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2345:
> 2343: if (!innerType.hasTag(CLASS) || !outerType.hasTag(CLASS))
> 2344: return false;
> 2345: innerType = erasure(innerType);
The javac way to write this would be either to compare types using
`Types.isSameType` or to compare the underlying
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Mon, 9 Jan 2023 15:03:10 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix incorrect @bug numbers in unit tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 85:
>
>> 83: *
>> 84: *
>> 85: * When tracking references, we distinguish between direct references
>> and indirect references,
>
> I'm trying to understand the code and it's not obvious to me as to where this
> difference comes into play. I believe (but I'm not sure) the spirit is to
> treat reference to `this` in the original constructor as "direct" while treat
> a reference to a parameter "a" in a method called from the constructor, where
> "a" is assigned "this", as indirect?
It's slightly different from that.
Considering any of the various things in scope that can point to an object
(these are: the current 'this' instance, the current outer 'this' instance,
method parameter/variable, method return value, top of Java stack), such a
thing has a "direct" reference if it might possibly _directly_ point to the
'this' we're tracking, while a thing has an "indirect" reference if it might
possibly point to the 'this' we're tracking through _at least one level of
indirection_.
This is just an attempt to eliminate some false positives by distinguishing
between those two cases. Originally I was going to try to track fields (in a
very limited way), and so this distinction was going to be more important, but
even without tracking fields it's still useful. For example, if some method
invokes `x.y.foo()` and `x` represents a direct but not an indirect 'this'
reference, then there is no leak declared.
Considering the other options... (a) if you only track direct references, then
you suffer from more false negatives (how many? unclear); (b) if you lump
direct and indirect references into one bucket, then you suffer from more false
positives (as in previous example `x.y.foo()`).
You can see an example of an indirect reference being tracked and exposed in
the unit test `ThisEscapeArrayElement.java`.
Another motivating example is lambdas. The act of simply _creating_ a lambda
never creates a leak, and a lambda never represents a _direct_ reference. But
it might represent an _indirect_ reference.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 1098:
>
>> 1096: private void visitLooped(T tree, Consumer
>> visitor) {
>> 1097: this.visitScoped(tree, false, t -> {
>> 1098: while (true) {
>
> Why is this needed? Can the tracking state of `this` change after multiple
> "execution" of the same code?
Yes, because the 'this' reference can bounce around through different variables
in scope each time around the loop. So we have to repeat the loop until all
'this' references have "flooded" into all the nooks and crannies.
The `ThisEscapeLoop.java` unit test demonstrates:
public class ThisEscapeLoop {
public ThisEscapeLoop() {
ThisEscapeLoop ref1 = this;
ThisEscapeLoop ref2 = null;
ThisEscapeLoop ref3 = null;
ThisEscapeLoop ref4 = null;
for (int i = 0; i < 100; i++) {
ref4 = ref3;
ref3 = ref2;
ref2 = ref1;
if (ref4 != null)
ref4.mightLeak();
}
}
public void mightLeak() {
}
}
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Mon, 9 Jan 2023 14:23:47 GMT, Maurizio Cimadamore
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix incorrect @bug numbers in unit tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2345:
>
>> 2343: if (!innerType.hasTag(CLASS) || !outerType.hasTag(CLASS))
>> 2344: return false;
>> 2345: innerType = erasure(innerType);
>
> The javac way to write this would be either to compare types using
> `Types.isSameType` or to compare the underlying class symbols with == (as
> class symbols are shared across multiple type instances).
OK I'm glad you pointed that out because I'm a little unclear on the best way
to do this bit.
Just to confirm, you are saying that this:
`if (erasure(type).equalsIgnoreMetadata(outerType)) {`
should be replaced with this?
`if (isSameType(type, outerType)) {`
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote:
>> The javadocs of the following methods used deprecated constructors of the
>> primitive wrapper classes:
>>
>> java.lang.ArrayStoreException
>> java.lang.ClassCastException
>> java.lang.Double.compare(double, double)
>> java.lang.Float.compare(float, float)
>> java.lang.Integer.getInteger(String, int)
>> java.lang.Integer.valueOf(String)
>> java.lang.Integer.valueOf(String, int)
>> java.lang.Long.getLong(String, long)
>> java.lang.Long.valueOf(String)
>> java.lang.Long.valueOf(String, int)
>> java.lang.Short.valueOf(String)
>> java.lang.Short.valueOf(String, int)
>>
>> This change replaces the constructors with .valueOf() methods except
>> java.lang.ClassCastException which was already fixed in JDK-8289730
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Include Byte in changes
I see your point about clarity; these snippets are not usage examples, but are
showing how the method could be implemented. I'd lean toward the auto-boxing
technique to keep the example to a minimum.
Since you are updating the example, perhaps they should use `@snippet
lang="java" { }` instead of the @code and blockquote markup.
-
PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:40:28 GMT, Justin Lu wrote: > > Given that type conversions create the primitive wrapper instances when > > they are needed, I think the examples should be reconsidered. They > > needlessly suggest that the wrapper instances need to be explicitly created. > > Hi Roger, thanks for the comment, > > When I was deciding between autoboxing and using the valueOf() methods, I > personally saw the latter as providing more clarity from a documentation > standpoint but I understand your concerns. Curious what others think on the > decision of using one vs the other. I concur that it is clearer to readers to have the explicit valueOf call in the code examples. - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v6]
> This PR adds a new lint warning category `this-escape`.
>
> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
> allow the JDK to continue to compile with `-Xlint:all`.
>
> A 'this' escape warning is generated for a constructor `A()` in a class `A`
> when the compiler detects that the following situation is _in theory
> possible:_
> * Some subclass `B extends A` exists, and `B` is defined in a separate source
> file (i.e., compilation unit)
> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
> * During the execution of `A()`, some non-static method of `B.foo()` could
> get invoked, perhaps indirectly
>
> In the above scenario, `B.foo()` would execute before `A()` has returned and
> before `B()` has performed any initialization. To the extent `B.foo()`
> accesses any fields of `B` - all of which are still uninitialized - it is
> likely to function incorrectly.
>
> Note, when determining if a 'this' escape is possible, the compiler makes no
> assumptions about code outside of the current compilation unit. It doesn't
> look outside of the current source file to see what might actually happen
> when a method is invoked. It does follow method and constructors within the
> current compilation unit, and applies a simplified
> union-of-all-possible-branches data flow analysis to see where 'this' could
> end up.
>
> From my review, virtually all of the warnings generated in the various JDK
> modules are valid warnings in the sense that a 'this' escape, as defined
> above, is really and truly possible. However, that doesn't imply that any
> bugs were found within the JDK - only that the possibility of a certain type
> of bug exists if certain superclass constructors are used by someone,
> somewhere, someday.
>
> For several "popular" classes, this PR also adds `@implNote`'s to the
> offending constructors so that subclass implementors are made aware of the
> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>
> More details and a couple of motivating examples are given in an included
> [doc
> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
> for some background.
>
> Ideally, over time the owners of the various modules would review their
> `@SuppressWarnings("this-escape")` annotations and determine which other
> constructors also warranted such an `@implNote`.
>
> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch of
> different JDK modules. My apologies for that. Adding these annotations was
> determined to be the more conservative approach, as compared to just
> excepting `this-escape` from various module builds globally.
>
> **Patch Navigation Guide**
>
> * Non-trivial compiler changes:
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>
> * Javadoc additions of `@implNote`:
>
> * `src/java.base/share/classes/java/io/PipedReader.java`
> * `src/java.base/share/classes/java/io/PipedWriter.java`
> * `src/java.base/share/classes/java/lang/Throwable.java`
> * `src/java.base/share/classes/java/util/ArrayDeque.java`
> * `src/java.base/share/classes/java/util/EnumMap.java`
> * `src/java.base/share/classes/java/util/HashSet.java`
> * `src/java.base/share/classes/java/util/Hashtable.java`
> * `src/java.base/share/classes/java/util/LinkedList.java`
> * `src/java.base/share/classes/java/util/TreeMap.java`
> * `src/java.base/share/classes/java/util/TreeSet.java`
>
> * New unit tests
> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>
> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
Archie L. Cobbs has updated the pull request incrementally with one additional
commit since the last revision:
Add this-escape to the Javadoc list of strings supported by @SuppressWarnings.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11874/files
- new: https://git.openjdk.org/jdk/pull/11874/files/7e2fdb07..d70d12f4
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=04-05
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/11874.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11874/head:
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote: >> The javadocs of the following methods used deprecated constructors of the >> primitive wrapper classes: >> >> java.lang.ArrayStoreException >> java.lang.ClassCastException >> java.lang.Double.compare(double, double) >> java.lang.Float.compare(float, float) >> java.lang.Integer.getInteger(String, int) >> java.lang.Integer.valueOf(String) >> java.lang.Integer.valueOf(String, int) >> java.lang.Long.getLong(String, long) >> java.lang.Long.valueOf(String) >> java.lang.Long.valueOf(String, int) >> java.lang.Short.valueOf(String) >> java.lang.Short.valueOf(String, int) >> >> This change replaces the constructors with .valueOf() methods except >> java.lang.ClassCastException which was already fixed in JDK-8289730 > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Include Byte in changes The code is used to show for example `getInteger(String nm, int val)` is equivalent to calling `getInteger(nm, Integer.valueOf(val))`. I agree with Justin that it's clearer to use the `valueOf` methods. In fact, `getInteger(String nam, int val)` example can't be represented with autoboxing. - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote: >> The javadocs of the following methods used deprecated constructors of the >> primitive wrapper classes: >> >> java.lang.ArrayStoreException >> java.lang.ClassCastException >> java.lang.Double.compare(double, double) >> java.lang.Float.compare(float, float) >> java.lang.Integer.getInteger(String, int) >> java.lang.Integer.valueOf(String) >> java.lang.Integer.valueOf(String, int) >> java.lang.Long.getLong(String, long) >> java.lang.Long.valueOf(String) >> java.lang.Long.valueOf(String, int) >> java.lang.Short.valueOf(String) >> java.lang.Short.valueOf(String, int) >> >> This change replaces the constructors with .valueOf() methods except >> java.lang.ClassCastException which was already fixed in JDK-8289730 > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Include Byte in changes Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote: >> The javadocs of the following methods used deprecated constructors of the >> primitive wrapper classes: >> >> java.lang.ArrayStoreException >> java.lang.ClassCastException >> java.lang.Double.compare(double, double) >> java.lang.Float.compare(float, float) >> java.lang.Integer.getInteger(String, int) >> java.lang.Integer.valueOf(String) >> java.lang.Integer.valueOf(String, int) >> java.lang.Long.getLong(String, long) >> java.lang.Long.valueOf(String) >> java.lang.Long.valueOf(String, int) >> java.lang.Short.valueOf(String) >> java.lang.Short.valueOf(String, int) >> >> This change replaces the constructors with .valueOf() methods except >> java.lang.ClassCastException which was already fixed in JDK-8289730 > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Include Byte in changes Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 18:02:22 GMT, Justin Lu wrote: >> The javadocs of the following methods used deprecated constructors of the >> primitive wrapper classes: >> >> java.lang.ArrayStoreException >> java.lang.ClassCastException >> java.lang.Double.compare(double, double) >> java.lang.Float.compare(float, float) >> java.lang.Integer.getInteger(String, int) >> java.lang.Integer.valueOf(String) >> java.lang.Integer.valueOf(String, int) >> java.lang.Long.getLong(String, long) >> java.lang.Long.valueOf(String) >> java.lang.Long.valueOf(String, int) >> java.lang.Short.valueOf(String) >> java.lang.Short.valueOf(String, int) >> >> This change replaces the constructors with .valueOf() methods except >> java.lang.ClassCastException which was already fixed in JDK-8289730 > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Include Byte in changes Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
On Tue, 10 Jan 2023 17:17:41 GMT, Naoto Sato wrote: > Fixing the subject method to recover gracefully on a failed > `ZoneRulesProvider` registration. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11928
Re: RFR: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
On Tue, 10 Jan 2023 17:17:41 GMT, Naoto Sato wrote: > Fixing the subject method to recover gracefully on a failed > `ZoneRulesProvider` registration. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11928
Re: RFR: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs [v2]
On Tue, 10 Jan 2023 19:59:36 GMT, Roger Riggs wrote:
> Since you are updating the example, perhaps they should use `@snippet
> lang="java" { }` instead of the @code and blockquote markup.
That's a good point and Lance brought that up as well, I will create a separate
issue to address instances where that change can be made.
-
PR: https://git.openjdk.org/jdk/pull/11912
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote: > `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been > updated to reflect the current state of Java and can be modernized: > > * Add `@Serial` annotations > * Seal classes and restrict subclassing for internal classes > * Use pattern matching for instance > * Remove redundant modifiers (such as some final declarations) > * Use Objects.requireNonNull for invariant checking > * Use diamond operators instead of explicit types > * Simplify expressions using Math::max ObMention @DougLea - PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote:
> `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been
> updated to reflect the current state of Java and can be modernized:
>
> * Add `@Serial` annotations
> * Seal classes and restrict subclassing for internal classes
> * Use pattern matching for instance
> * Remove redundant modifiers (such as some final declarations)
> * Use Objects.requireNonNull for invariant checking
> * Use diamond operators instead of explicit types
> * Simplify expressions using Math::max
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
104:
> 102: * {@code size}, {@code isEmpty}, and {@code containsValue} are typically
> 103: * useful only when a map is not undergoing concurrent updates in other
> threads.
> 104: * Otherwise, the results of these methods reflect transient states
I agree the added comma here is an improvement.
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
357:
> 355: * sometimes deviate significantly from uniform randomness. This
> 356: * includes the case when N > (1<<30), so some keys MUST collide.
> 357: * Similarly, for dumb or hostile usages in which multiple keys are
"Similarly" should almost always be followed by a comma, but this is an
exception.
-
PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote:
> `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been
> updated to reflect the current state of Java and can be modernized:
>
> * Add `@Serial` annotations
> * Seal classes and restrict subclassing for internal classes
> * Use pattern matching for instance
> * Remove redundant modifiers (such as some final declarations)
> * Use Objects.requireNonNull for invariant checking
> * Use diamond operators instead of explicit types
> * Simplify expressions using Math::max
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
629:
> 627: * exported). Otherwise, keys and vals are never null.
> 628: */
> 629: static sealed class Node implements Map.Entry {
There is a long term modernization task of making nested classes private to
make use of
https://openjdk.org/jeps/181
It looks like this is an example.
-
PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
On Tue, 10 Jan 2023 17:17:41 GMT, Naoto Sato wrote: > Fixing the subject method to recover gracefully on a failed > `ZoneRulesProvider` registration. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11928
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote: > `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been > updated to reflect the current state of Java and can be modernized: > > * Add `@Serial` annotations > * Seal classes and restrict subclassing for internal classes > * Use pattern matching for instance > * Remove redundant modifiers (such as some final declarations) > * Use Objects.requireNonNull for invariant checking > * Use diamond operators instead of explicit types > * Simplify expressions using Math::max src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line 2002: > 2000: } > 2001: if (delta != 0) > 2002: addCount((long)delta, binCount); Not sure why delta is not declared long as in clear(). But if not, then the explicit cast is documentation of the implementation choice to declare it int. - PR: https://git.openjdk.org/jdk/pull/11924
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Tue, 10 Jan 2023 19:42:06 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2345:
>>
>>> 2343: if (!innerType.hasTag(CLASS) || !outerType.hasTag(CLASS))
>>> 2344: return false;
>>> 2345: innerType = erasure(innerType);
>>
>> The javac way to write this would be either to compare types using
>> `Types.isSameType` or to compare the underlying class symbols with == (as
>> class symbols are shared across multiple type instances).
>
> OK I'm glad you pointed that out because I'm a little unclear on the best way
> to do this bit.
>
> Just to confirm, you are saying that this:
>
> `if (erasure(type).equalsIgnoreMetadata(outerType)) {`
>
> should be replaced with this?
>
> `if (isSameType(type, outerType)) {`
yes
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Tue, 10 Jan 2023 19:18:04 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 85:
>>
>>> 83: *
>>> 84: *
>>> 85: * When tracking references, we distinguish between direct references
>>> and indirect references,
>>
>> I'm trying to understand the code and it's not obvious to me as to where
>> this difference comes into play. I believe (but I'm not sure) the spirit is
>> to treat reference to `this` in the original constructor as "direct" while
>> treat a reference to a parameter "a" in a method called from the
>> constructor, where "a" is assigned "this", as indirect?
>
> It's slightly different from that.
>
> Considering any of the various things in scope that can point to an object
> (these are: the current 'this' instance, the current outer 'this' instance,
> method parameter/variable, method return value, top of Java stack), such a
> thing has a "direct" reference if it might possibly _directly_ point to the
> 'this' we're tracking, while a thing has an "indirect" reference if it might
> possibly point to the 'this' we're tracking through _at least one level of
> indirection_.
>
> This is just an attempt to eliminate some false positives by distinguishing
> between those two cases. Originally I was going to try to track fields (in a
> very limited way), and so this distinction was going to be more important,
> but even without tracking fields it's still useful. For example, if some
> method invokes `x.y.foo()` and `x` represents a direct but not an indirect
> 'this' reference, then there is no leak declared.
>
> Considering the other options... (a) if you only track direct references,
> then you suffer from more false negatives (how many? unclear); (b) if you
> lump direct and indirect references into one bucket, then you suffer from
> more false positives (as in previous example `x.y.foo()`).
>
> You can see an example of an indirect reference being tracked and exposed in
> the unit test `ThisEscapeArrayElement.java`.
>
> Another motivating example is lambdas. The act of simply _creating_ a lambda
> never creates a leak, and a lambda never represents a _direct_ reference. But
> it might represent an _indirect_ reference.
So, if I understand correctly your array element test, when we have `new
Object[][] { { this } }` the analysis is able to detect that there might be
some direct reference nested inside the array. So the outer array is an
indirect reference. The inner array is also an indirect reference - but any
element accessed on the inner array are direct reference - and so you detect a
leak there. Correct?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Tue, 10 Jan 2023 23:45:59 GMT, Maurizio Cimadamore
wrote:
>> It's slightly different from that.
>>
>> Considering any of the various things in scope that can point to an object
>> (these are: the current 'this' instance, the current outer 'this' instance,
>> method parameter/variable, method return value, top of Java stack), such a
>> thing has a "direct" reference if it might possibly _directly_ point to the
>> 'this' we're tracking, while a thing has an "indirect" reference if it might
>> possibly point to the 'this' we're tracking through _at least one level of
>> indirection_.
>>
>> This is just an attempt to eliminate some false positives by distinguishing
>> between those two cases. Originally I was going to try to track fields (in a
>> very limited way), and so this distinction was going to be more important,
>> but even without tracking fields it's still useful. For example, if some
>> method invokes `x.y.foo()` and `x` represents a direct but not an indirect
>> 'this' reference, then there is no leak declared.
>>
>> Considering the other options... (a) if you only track direct references,
>> then you suffer from more false negatives (how many? unclear); (b) if you
>> lump direct and indirect references into one bucket, then you suffer from
>> more false positives (as in previous example `x.y.foo()`).
>>
>> You can see an example of an indirect reference being tracked and exposed in
>> the unit test `ThisEscapeArrayElement.java`.
>>
>> Another motivating example is lambdas. The act of simply _creating_ a lambda
>> never creates a leak, and a lambda never represents a _direct_ reference.
>> But it might represent an _indirect_ reference.
>
> So, if I understand correctly your array element test, when we have `new
> Object[][] { { this } }` the analysis is able to detect that there might be
> some direct reference nested inside the array. So the outer array is an
> indirect reference. The inner array is also an indirect reference - but any
> element accessed on the inner array are direct reference - and so you detect
> a leak there. Correct?
Yes - although it's not tracked being tracked any more precisely than "one or
more levels of indirection".
And of course by "reference" we only mean "the possibility of a reference" - in
general we don't know for sure.
Here's the logic:
* The analysis knows `this` is a direct reference
* Therefore the array expression `{ this }` has an indirect reference (but no
direct reference)
* The outer array expression `{ { this } }` therefore _also_ has an indirect
reference (but no direct reference)
At this point, we don't know how many levels of indirection there are in
`array` though only that its ≥ 1.
* There expression `array[0]` therefore is determined to have both direct and
indirect references (they are both _possible_ because we've dereferenced
something with an indirect reference)
* As does `array[0][0]` for the same reason
So invoking `array[0][0].hashCode()` means invoking `hashCode()` on something
that has a possible direct reference, and is therefore a leak.
Note that because of the imprecision in the number of levels of indirection,
invoking `array[0].hashCode()` would also have generated a warning - but in
that case, a false positive.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Tue, 10 Jan 2023 19:20:35 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 1098:
>>
>>> 1096: private void visitLooped(T tree, Consumer
>>> visitor) {
>>> 1097: this.visitScoped(tree, false, t -> {
>>> 1098: while (true) {
>>
>> Why is this needed? Can the tracking state of `this` change after multiple
>> "execution" of the same code?
>
> Yes, because the 'this' reference can bounce around through different
> variables in scope each time around the loop. So we have to repeat the loop
> until all 'this' references have "flooded" into all the nooks and crannies.
>
> The `ThisEscapeLoop.java` unit test demonstrates:
>
> public class ThisEscapeLoop {
>
> public ThisEscapeLoop() {
> ThisEscapeLoop ref1 = this;
> ThisEscapeLoop ref2 = null;
> ThisEscapeLoop ref3 = null;
> ThisEscapeLoop ref4 = null;
> for (int i = 0; i < 100; i++) {
> ref4 = ref3;
> ref3 = ref2;
> ref2 = ref1;
> if (ref4 != null)
> ref4.mightLeak();
> }
> }
>
> public void mightLeak() {
> }
> }
So, if the code was be like this:
ThisEscapeLoop ref11 = this;
ThisEscapeLoop ref12 = null;
ThisEscapeLoop ref13 = null;
ThisEscapeLoop ref14 = null;
for (int i = 0; i < 100; i++) {
ref14 = ref13;
ref13 = ref12;
ref12 = ref11;
ThisEscapeLoop ref21 = ref14;
ThisEscapeLoop ref22 = null;
ThisEscapeLoop ref23 = null;
ThisEscapeLoop ref24 = null;
for (int i = 0; i < 100; i++) {
ref24 = ref23;
ref23 = ref22;
ref22 = ref21;
if (ref24 != null)
ref24.mightLeak();
}
}
Then it would take not 3 iterations but 3 * 3 to figure out that it is a
potential leak?
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Wed, 11 Jan 2023 00:04:14 GMT, Maurizio Cimadamore
wrote:
>> Yes, because the 'this' reference can bounce around through different
>> variables in scope each time around the loop. So we have to repeat the loop
>> until all 'this' references have "flooded" into all the nooks and crannies.
>>
>> The `ThisEscapeLoop.java` unit test demonstrates:
>>
>> public class ThisEscapeLoop {
>>
>> public ThisEscapeLoop() {
>> ThisEscapeLoop ref1 = this;
>> ThisEscapeLoop ref2 = null;
>> ThisEscapeLoop ref3 = null;
>> ThisEscapeLoop ref4 = null;
>> for (int i = 0; i < 100; i++) {
>> ref4 = ref3;
>> ref3 = ref2;
>> ref2 = ref1;
>> if (ref4 != null)
>> ref4.mightLeak();
>> }
>> }
>>
>> public void mightLeak() {
>> }
>> }
>
> So, if the code was be like this:
>
>
> ThisEscapeLoop ref11 = this;
> ThisEscapeLoop ref12 = null;
> ThisEscapeLoop ref13 = null;
> ThisEscapeLoop ref14 = null;
> for (int i = 0; i < 100; i++) {
> ref14 = ref13;
> ref13 = ref12;
> ref12 = ref11;
> ThisEscapeLoop ref21 = ref14;
> ThisEscapeLoop ref22 = null;
> ThisEscapeLoop ref23 = null;
> ThisEscapeLoop ref24 = null;
> for (int i = 0; i < 100; i++) {
> ref24 = ref23;
> ref23 = ref22;
> ref22 = ref21;
> if (ref24 != null)
> ref24.mightLeak();
> }
> }
>
>
> Then it would take not 3 iterations but 3 * 3 to figure out that it is a
> potential leak?
Actually I think it would take 1 + 1 + 3 iterations.
During the first two iterations of the outer loop, nothing changes after the
first go round of the inner loop - i.e., the total set of possible references
in existence does not change, because all of the assignments in the inner loop
won't involve any 'this' references.
It's only the during the third iteration of the outer loop that any 'this'
references seep into any of the variables seen by the inner loop. Then it will
take 3 cycles for the reference set to converge again.
However, this is not to say that there aren't some pathological examples out
there. I guess the question is could they exist in "normal" code.
-
PR: https://git.openjdk.org/jdk/pull/11874
[jdk20] RFR: 8299090: Specify coordinate order for additional CaptureCallState parameters on class as well
A small doc clarification that also specifies where the additional MemorySegment parameter of a downcall method handle linked with the captureCallState option appears in the parameter list, on the CaptureCallState class. - Commit messages: - add parameter order clarification Changes: https://git.openjdk.org/jdk20/pull/95/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=95&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299090 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk20/pull/95.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/95/head:pull/95 PR: https://git.openjdk.org/jdk20/pull/95
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
> This PR adds a new lint warning category `this-escape`.
>
> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
> allow the JDK to continue to compile with `-Xlint:all`.
>
> A 'this' escape warning is generated for a constructor `A()` in a class `A`
> when the compiler detects that the following situation is _in theory
> possible:_
> * Some subclass `B extends A` exists, and `B` is defined in a separate source
> file (i.e., compilation unit)
> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
> * During the execution of `A()`, some non-static method of `B.foo()` could
> get invoked, perhaps indirectly
>
> In the above scenario, `B.foo()` would execute before `A()` has returned and
> before `B()` has performed any initialization. To the extent `B.foo()`
> accesses any fields of `B` - all of which are still uninitialized - it is
> likely to function incorrectly.
>
> Note, when determining if a 'this' escape is possible, the compiler makes no
> assumptions about code outside of the current compilation unit. It doesn't
> look outside of the current source file to see what might actually happen
> when a method is invoked. It does follow method and constructors within the
> current compilation unit, and applies a simplified
> union-of-all-possible-branches data flow analysis to see where 'this' could
> end up.
>
> From my review, virtually all of the warnings generated in the various JDK
> modules are valid warnings in the sense that a 'this' escape, as defined
> above, is really and truly possible. However, that doesn't imply that any
> bugs were found within the JDK - only that the possibility of a certain type
> of bug exists if certain superclass constructors are used by someone,
> somewhere, someday.
>
> For several "popular" classes, this PR also adds `@implNote`'s to the
> offending constructors so that subclass implementors are made aware of the
> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>
> More details and a couple of motivating examples are given in an included
> [doc
> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
> for some background.
>
> Ideally, over time the owners of the various modules would review their
> `@SuppressWarnings("this-escape")` annotations and determine which other
> constructors also warranted such an `@implNote`.
>
> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch of
> different JDK modules. My apologies for that. Adding these annotations was
> determined to be the more conservative approach, as compared to just
> excepting `this-escape` from various module builds globally.
>
> **Patch Navigation Guide**
>
> * Non-trivial compiler changes:
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
> *
> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>
> * Javadoc additions of `@implNote`:
>
> * `src/java.base/share/classes/java/io/PipedReader.java`
> * `src/java.base/share/classes/java/io/PipedWriter.java`
> * `src/java.base/share/classes/java/lang/Throwable.java`
> * `src/java.base/share/classes/java/util/ArrayDeque.java`
> * `src/java.base/share/classes/java/util/EnumMap.java`
> * `src/java.base/share/classes/java/util/HashSet.java`
> * `src/java.base/share/classes/java/util/Hashtable.java`
> * `src/java.base/share/classes/java/util/LinkedList.java`
> * `src/java.base/share/classes/java/util/TreeMap.java`
> * `src/java.base/share/classes/java/util/TreeSet.java`
>
> * New unit tests
> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>
> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
Archie L. Cobbs has updated the pull request incrementally with two additional
commits since the last revision:
- Use the more appropriate Type comparison method Types.isSameType().
- Add some more comments to clarify how the analysis works.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11874/files
- new: https://git.openjdk.org/jdk/pull/11874/files/d70d12f4..6e96a7d7
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=05-06
Stats: 31 lines in 2 files changed: 16 ins; 10 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/11874.diff
Fetc
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]
On Tue, 10 Jan 2023 23:38:14 GMT, Maurizio Cimadamore
wrote:
>> OK I'm glad you pointed that out because I'm a little unclear on the best
>> way to do this bit.
>>
>> Just to confirm, you are saying that this:
>>
>> `if (erasure(type).equalsIgnoreMetadata(outerType)) {`
>>
>> should be replaced with this?
>>
>> `if (isSameType(type, outerType)) {`
>
> yes
Thanks... updated in `6e96a7d76f8`.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC src/java.base/share/classes/java/io/ObjectStreamConstants.java line 123: > 121: * new Proxy Class Descriptor. > 122: */ > 123: static byte TC_PROXYCLASSDESC = (byte)0x7D; alignment is confusing here. As we touch every constant here I suggest to realign all values properly. (Or remove alignment attempt completely.) - PR: https://git.openjdk.org/jdk/pull/11848
