Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v7]
On Wed, 10 Jan 2024 18:27:45 GMT, Pavel Rappo wrote: > > /contributor add @pavelrappo > > Thanks, Joe. Making me an "overriding author" was a bit over the top. :) Skimmed the docs too quickly when looking for a "co-author" command :-) - PR Comment: https://git.openjdk.org/jdk/pull/17239#issuecomment-1885410390
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v7]
On Wed, 10 Jan 2024 18:16:05 GMT, Joe Darcy wrote: >> As recently discussed on core libs, sealed-ness information could be >> included in the Class.toGenericString() output, analagous to how "modifiers" >> that also correspond to JVM access flags are handled. >> >> This is the initial spec, implementation, and test updated needed for that >> change. If there is consensus this is a reasonable direction, I'll create >> the CSR, etc. > > Joe Darcy 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 eight additional commits since > the last revision: > > - Adjust JLS quote and update copyright. > - Merge branch 'master' into JDK-8322878 > - Expand test cases. > - Update spec; test sealed class structures. > - Improve regression test. > - Update non-sealed computation. > - Add non-seal'ing support. > - JDK-8322878: Including sealing information Class.toGenericString() Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17239#pullrequestreview-1813866824
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v7]
On Wed, 10 Jan 2024 18:16:05 GMT, Joe Darcy wrote: >> As recently discussed on core libs, sealed-ness information could be >> included in the Class.toGenericString() output, analagous to how "modifiers" >> that also correspond to JVM access flags are handled. >> >> This is the initial spec, implementation, and test updated needed for that >> change. If there is consensus this is a reasonable direction, I'll create >> the CSR, etc. > > Joe Darcy 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 eight additional commits since > the last revision: > > - Adjust JLS quote and update copyright. > - Merge branch 'master' into JDK-8322878 > - Expand test cases. > - Update spec; test sealed class structures. > - Improve regression test. > - Update non-sealed computation. > - Add non-seal'ing support. > - JDK-8322878: Including sealing information Class.toGenericString() > /contributor add @pavelrappo Thanks, Joe. Making me an "overriding author" was a bit over the top. :) - PR Comment: https://git.openjdk.org/jdk/pull/17239#issuecomment-1885397723
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v7]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy 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 eight additional commits since the last revision: - Adjust JLS quote and update copyright. - Merge branch 'master' into JDK-8322878 - Expand test cases. - Update spec; test sealed class structures. - Improve regression test. - Update non-sealed computation. - Add non-seal'ing support. - JDK-8322878: Including sealing information Class.toGenericString() - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/d5a451d0..4cfc4e3d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=05-06 Stats: 17609 lines in 565 files changed: 12491 ins; 2504 del; 2614 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v6]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Expand test cases. - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/2d02d9b1..d5a451d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=04-05 Stats: 78 lines in 1 file changed: 74 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v5]
On Tue, 9 Jan 2024 19:37:54 GMT, Joe Darcy wrote: >> As recently discussed on core libs, sealed-ness information could be >> included in the Class.toGenericString() output, analagous to how "modifiers" >> that also correspond to JVM access flags are handled. >> >> This is the initial spec, implementation, and test updated needed for that >> change. If there is consensus this is a reasonable direction, I'll create >> the CSR, etc. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update spec; test sealed class structures. The updates to add non-sealed look fine. The CSR needs an update to the Solution section to mention non-sealed. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17239#pullrequestreview-1811897603
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v5]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update spec; test sealed class structures. - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/e1efbf30..2d02d9b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=03-04 Stats: 52 lines in 2 files changed: 46 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v4]
On Mon, 8 Jan 2024 22:29:47 GMT, Joe Darcy wrote:
>>> Since it doesn't seem possible to do so, I did not attempt to relay
>>> "non-sealed" information in this PR :-)
>>
>> Naively, I thought that something like this is possible _in principle_; I
>> might be mistaken though:
>>
>> diff --git a/src/java.base/share/classes/java/lang/Class.java
>> b/src/java.base/share/classes/java/lang/Class.java
>> index 851d65d06ad..014845860d0 100644
>> --- a/src/java.base/share/classes/java/lang/Class.java
>> +++ b/src/java.base/share/classes/java/lang/Class.java
>> @@ -4771,6 +4771,30 @@ public boolean isSealed() {
>> return getPermittedSubclasses() != null;
>> }
>>
>> +private boolean isNonSealed() {
>> +if (isSealed())
>> +return false;
>> +if (!isInterface() && Modifier.isFinal(getModifiers())) {
>> +// unlike interface, class can be final
>> +return false;
>> +}
>> +// if an ancestor is sealed, this class can either be non-sealed or
>> final
>> +return hasSealedAncestor(this);
>> +}
>> +
>> +private boolean hasSealedAncestor(Class clazz) {
>> +var superclass = clazz.getSuperclass();
>> +if (superclass != null) {
>> +if (superclass.isSealed() || hasSealedAncestor(superclass))
>> +return true;
>> +}
>> +for (var superinterface : clazz.getInterfaces()) {
>> +if (superinterface.isSealed() ||
>> hasSealedAncestor(superinterface))
>> +return true;
>> +}
>> +return false;
>> +}
>> +
>> private native Class[] getPermittedSubclasses0();
>>
>> /*
>
>> Thanks @pavelrappo; I'll explore incorporating functionality like this into
>> the PR, probably next week.
>
> Pushed an initial cut of non-sealing support; will add more test cases later.
Thanks for including `non-sealed`, Joe.
Regarding your implementation: you are right that only the direct superclass
and direct superinterfaces need to be explored for being `sealed`. No need to
explore the full ancestry. So, recursion in my [initial proposal] was
superfluous.
That said, I have a question on [JLS 8.1.1.2], which you copied as inline
commentary:
> It is a compile-time error if a class is declared non-sealed but has neither
> a sealed direct superclass nor a sealed direct superinterface.
>
> Thus, a subclass of a non-sealed class cannot itself be declared non-sealed.
I wonder if it can be clarified to account for hierarchies where a `non-sealed`
class or `non-sealed` interface has a `non-sealed` direct superinterface, or a
`non-sealed` class has a `non-sealed` direct superclass.
For example:
sealed interface A permits B { }
sealed interface I permits X { }
non-sealed interface B extends A { }
non-sealed interface X extends B, I { }
and
sealed class A permits B { }
sealed interface I permits C { }
non-sealed class B extends A { }
non-sealed class C extends B implements I { }
(CC'ing @GavinBierman)
[initial proposal]:
https://github.com/openjdk/jdk/pull/17239#discussion_r1440867004
[JLS 8.1.1.2]:
https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1.2
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1446243516
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v3]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update non-sealed computation. - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/7222d78d..ac897d9b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=01-02 Stats: 22 lines in 1 file changed: 16 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v4]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Improve regression test. - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/ac897d9b..e1efbf30 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=02-03 Stats: 24 lines in 1 file changed: 9 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v2]
> As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Add non-seal'ing support. - Changes: - all: https://git.openjdk.org/jdk/pull/17239/files - new: https://git.openjdk.org/jdk/pull/17239/files/81084835..7222d78d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=00-01 Stats: 42 lines in 2 files changed: 34 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString() [v2]
On Wed, 3 Jan 2024 19:44:51 GMT, Pavel Rappo wrote:
>> I think the best place, or least-bad place, to discuss the "modifier"
>> ordering of sealed/non-sealed would be an informative note on
>> Modifier.toString(int) -- "The sealed/non-sealed Java language modifiers are
>> not represented in the class file as access flags and thus not modeled by
>> this class [java.lang.reflect.Modifier] sealed/non-sealed should be
>> presented in the same location as final."
>>
>> Since it doesn't seem possible to do so, I did not attempt to relay
>> "non-sealed" information in this PR :-)
>
>> Since it doesn't seem possible to do so, I did not attempt to relay
>> "non-sealed" information in this PR :-)
>
> Naively, I thought that something like this is possible _in principle_; I
> might be mistaken though:
>
> diff --git a/src/java.base/share/classes/java/lang/Class.java
> b/src/java.base/share/classes/java/lang/Class.java
> index 851d65d06ad..014845860d0 100644
> --- a/src/java.base/share/classes/java/lang/Class.java
> +++ b/src/java.base/share/classes/java/lang/Class.java
> @@ -4771,6 +4771,30 @@ public boolean isSealed() {
> return getPermittedSubclasses() != null;
> }
>
> +private boolean isNonSealed() {
> +if (isSealed())
> +return false;
> +if (!isInterface() && Modifier.isFinal(getModifiers())) {
> +// unlike interface, class can be final
> +return false;
> +}
> +// if an ancestor is sealed, this class can either be non-sealed or
> final
> +return hasSealedAncestor(this);
> +}
> +
> +private boolean hasSealedAncestor(Class clazz) {
> +var superclass = clazz.getSuperclass();
> +if (superclass != null) {
> +if (superclass.isSealed() || hasSealedAncestor(superclass))
> +return true;
> +}
> +for (var superinterface : clazz.getInterfaces()) {
> +if (superinterface.isSealed() ||
> hasSealedAncestor(superinterface))
> +return true;
> +}
> +return false;
> +}
> +
> private native Class[] getPermittedSubclasses0();
>
> /*
> Thanks @pavelrappo; I'll explore incorporating functionality like this into
> the PR, probably next week.
Pushed an initial cut of non-sealing support; will add more test cases later.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1445420505
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 22:20:15 GMT, Joe Darcy wrote:
>>> Since it doesn't seem possible to do so, I did not attempt to relay
>>> "non-sealed" information in this PR :-)
>>
>> Naively, I thought that something like this is possible _in principle_; I
>> might be mistaken though:
>>
>> diff --git a/src/java.base/share/classes/java/lang/Class.java
>> b/src/java.base/share/classes/java/lang/Class.java
>> index 851d65d06ad..014845860d0 100644
>> --- a/src/java.base/share/classes/java/lang/Class.java
>> +++ b/src/java.base/share/classes/java/lang/Class.java
>> @@ -4771,6 +4771,30 @@ public boolean isSealed() {
>> return getPermittedSubclasses() != null;
>> }
>>
>> +private boolean isNonSealed() {
>> +if (isSealed())
>> +return false;
>> +if (!isInterface() && Modifier.isFinal(getModifiers())) {
>> +// unlike interface, class can be final
>> +return false;
>> +}
>> +// if an ancestor is sealed, this class can either be non-sealed or
>> final
>> +return hasSealedAncestor(this);
>> +}
>> +
>> +private boolean hasSealedAncestor(Class clazz) {
>> +var superclass = clazz.getSuperclass();
>> +if (superclass != null) {
>> +if (superclass.isSealed() || hasSealedAncestor(superclass))
>> +return true;
>> +}
>> +for (var superinterface : clazz.getInterfaces()) {
>> +if (superinterface.isSealed() ||
>> hasSealedAncestor(superinterface))
>> +return true;
>> +}
>> +return false;
>> +}
>> +
>> private native Class[] getPermittedSubclasses0();
>>
>> /*
>
> Thanks @pavelrappo; I'll explore incorporating functionality like this into
> the PR, probably next week.
> I think the best place, or least-bad place, to discuss the "modifier"
> ordering of sealed/non-sealed would be an informative note on
> Modifier.toString(int) -- "The sealed/non-sealed Java language modifiers are
> not represented in the class file as access flags and thus not modeled by
> this class [java.lang.reflect.Modifier] sealed/non-sealed should be
> presented in the same location as final."
Filed JDK-8322979: Add informative discussion to Modifier.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1441050656
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 19:44:51 GMT, Pavel Rappo wrote:
>> I think the best place, or least-bad place, to discuss the "modifier"
>> ordering of sealed/non-sealed would be an informative note on
>> Modifier.toString(int) -- "The sealed/non-sealed Java language modifiers are
>> not represented in the class file as access flags and thus not modeled by
>> this class [java.lang.reflect.Modifier] sealed/non-sealed should be
>> presented in the same location as final."
>>
>> Since it doesn't seem possible to do so, I did not attempt to relay
>> "non-sealed" information in this PR :-)
>
>> Since it doesn't seem possible to do so, I did not attempt to relay
>> "non-sealed" information in this PR :-)
>
> Naively, I thought that something like this is possible _in principle_; I
> might be mistaken though:
>
> diff --git a/src/java.base/share/classes/java/lang/Class.java
> b/src/java.base/share/classes/java/lang/Class.java
> index 851d65d06ad..014845860d0 100644
> --- a/src/java.base/share/classes/java/lang/Class.java
> +++ b/src/java.base/share/classes/java/lang/Class.java
> @@ -4771,6 +4771,30 @@ public boolean isSealed() {
> return getPermittedSubclasses() != null;
> }
>
> +private boolean isNonSealed() {
> +if (isSealed())
> +return false;
> +if (!isInterface() && Modifier.isFinal(getModifiers())) {
> +// unlike interface, class can be final
> +return false;
> +}
> +// if an ancestor is sealed, this class can either be non-sealed or
> final
> +return hasSealedAncestor(this);
> +}
> +
> +private boolean hasSealedAncestor(Class clazz) {
> +var superclass = clazz.getSuperclass();
> +if (superclass != null) {
> +if (superclass.isSealed() || hasSealedAncestor(superclass))
> +return true;
> +}
> +for (var superinterface : clazz.getInterfaces()) {
> +if (superinterface.isSealed() ||
> hasSealedAncestor(superinterface))
> +return true;
> +}
> +return false;
> +}
> +
> private native Class[] getPermittedSubclasses0();
>
> /*
Thanks @pavelrappo; I'll explore incorporating functionality like this into the
PR, probably next week.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1441020362
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 18:16:24 GMT, Joe Darcy wrote:
> Since it doesn't seem possible to do so, I did not attempt to relay
> "non-sealed" information in this PR :-)
Naively, I thought that something like this is possible _in principle_; I might
be mistaken though:
diff --git a/src/java.base/share/classes/java/lang/Class.java
b/src/java.base/share/classes/java/lang/Class.java
index 851d65d06ad..014845860d0 100644
--- a/src/java.base/share/classes/java/lang/Class.java
+++ b/src/java.base/share/classes/java/lang/Class.java
@@ -4771,6 +4771,30 @@ public boolean isSealed() {
return getPermittedSubclasses() != null;
}
+private boolean isNonSealed() {
+if (isSealed())
+return false;
+if (!isInterface() && Modifier.isFinal(getModifiers())) {
+// unlike interface, class can be final
+return false;
+}
+// if an ancestor is sealed, this class can either be non-sealed or
final
+return hasSealedAncestor(this);
+}
+
+private boolean hasSealedAncestor(Class clazz) {
+var superclass = clazz.getSuperclass();
+if (superclass != null) {
+if (superclass.isSealed() || hasSealedAncestor(superclass))
+return true;
+}
+for (var superinterface : clazz.getInterfaces()) {
+if (superinterface.isSealed() || hasSealedAncestor(superinterface))
+return true;
+}
+return false;
+}
+
private native Class[] getPermittedSubclasses0();
/*
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440867004
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 06:43:22 GMT, Joe Darcy wrote: > As recently discussed on core libs, sealed-ness information could be included > in the Class.toGenericString() output, analagous to how "modifiers" that also > correspond to JVM access flags are handled. > > This is the initial spec, implementation, and test updated needed for that > change. If there is consensus this is a reasonable direction, I'll create the > CSR, etc. As the output of toGenericString does not attempt to fully capture the full declaration information of a class or interfaces, omitted any extends or implementation information for example, I think it is fine to also omit any listed of permitted subtypes for a sealed class or interface. This sidesteps design issues around which permitted subtypes should be filtered out for display purposes, etc. CSR up for review: https://bugs.openjdk.org/browse/JDK-8322969 TIA - PR Comment: https://git.openjdk.org/jdk/pull/17239#issuecomment-1875783748 PR Comment: https://git.openjdk.org/jdk/pull/17239#issuecomment-1875794166
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 16:40:32 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/Class.java line 264:
>>
>>> 262: /**
>>> 263: * Returns a string describing this {@code Class}, including
>>> 264: * information about modifiers, {@linkplain #isSealed() sealing},
>>> and type parameters.
>>
>> If Class.toGenericString is a useful API point for describing the blessed
>> order of modifiers in the JDK
>> perhaps the existing classes that describe modifiers could/should cross
>> reference this method.
>
> Given potential lossiness of source modifiers presentation by
> java.lang.reflect, I don't think this method is a good host for describing
> the canonical modifier order. Mentioning? Probably. Describing? No.
>
> Separately, but also related: while it does not seem impossible, this PR does
> not implement `non-sealed`.
I think the best place, or least-bad place, to discuss the "modifier" ordering
of sealed/non-sealed would be an informative note on Modifier.toString(int) --
"The sealed/non-sealed Java language modifiers are not represented in the class
file as access flags and thus not modeled by this class
[java.lang.reflect.Modifier] sealed/non-sealed should be presented in the
same location as final."
Since it doesn't seem possible to do so, I did not attempt to relay
"non-sealed" information in this PR :-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440765048
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 14:52:48 GMT, Roger Riggs wrote: >> As recently discussed on core libs, sealed-ness information could be >> included in the Class.toGenericString() output, analagous to how "modifiers" >> that also correspond to JVM access flags are handled. >> >> This is the initial spec, implementation, and test updated needed for that >> change. If there is consensus this is a reasonable direction, I'll create >> the CSR, etc. > > src/java.base/share/classes/java/lang/Class.java line 320: > >> 318: // sufficient to check for sealed-ness after all >> 319: // modifiers are printed. >> 320: boolean isSealed = isSealed(); > > A small concern about performance. > Class.isSealed() calls getPermittedSubclasses() which assembles and filters > the list of classes and a possible security manager check and then only > checks for non-null. > Perhaps a separate issue could look at that. If Class.toGenericString() turned out to be performance sensitive, which would be a surprise, then we could look into alternate implementations. - PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440757367
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 14:59:33 GMT, Roger Riggs wrote:
>> As recently discussed on core libs, sealed-ness information could be
>> included in the Class.toGenericString() output, analagous to how "modifiers"
>> that also correspond to JVM access flags are handled.
>>
>> This is the initial spec, implementation, and test updated needed for that
>> change. If there is consensus this is a reasonable direction, I'll create
>> the CSR, etc.
>
> src/java.base/share/classes/java/lang/Class.java line 264:
>
>> 262: /**
>> 263: * Returns a string describing this {@code Class}, including
>> 264: * information about modifiers, {@linkplain #isSealed() sealing},
>> and type parameters.
>
> If Class.toGenericString is a useful API point for describing the blessed
> order of modifiers in the JDK
> perhaps the existing classes that describe modifiers could/should cross
> reference this method.
Given potential lossiness of source modifiers presentation by
java.lang.reflect, I don't think this method is a good host for describing the
canonical modifier order. Mentioning? Probably. Describing? No.
Separately, but also related: while it does not seem impossible, this PR does
not implement `non-sealed`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440663061
Re: RFR: JDK-8322878: Including sealing information Class.toGenericString()
On Wed, 3 Jan 2024 06:43:22 GMT, Joe Darcy wrote:
> As recently discussed on core libs, sealed-ness information could be included
> in the Class.toGenericString() output, analagous to how "modifiers" that also
> correspond to JVM access flags are handled.
>
> This is the initial spec, implementation, and test updated needed for that
> change. If there is consensus this is a reasonable direction, I'll create the
> CSR, etc.
LGTM
src/java.base/share/classes/java/lang/Class.java line 264:
> 262: /**
> 263: * Returns a string describing this {@code Class}, including
> 264: * information about modifiers, {@linkplain #isSealed() sealing},
> and type parameters.
If Class.toGenericString is a useful API point for describing the blessed order
of modifiers in the JDK
perhaps the existing classes that describe modifiers could/should cross
reference this method.
src/java.base/share/classes/java/lang/Class.java line 320:
> 318: // sufficient to check for sealed-ness after all
> 319: // modifiers are printed.
> 320: boolean isSealed = isSealed();
A small concern about performance.
Class.isSealed() calls getPermittedSubclasses() which assembles and filters the
list of classes and a possible security manager check and then only checks for
non-null.
Perhaps a separate issue could look at that.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17239#pullrequestreview-1802318323
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440551717
PR Review Comment: https://git.openjdk.org/jdk/pull/17239#discussion_r1440543767
RFR: JDK-8322878: Including sealing information Class.toGenericString()
As recently discussed on core libs, sealed-ness information could be included in the Class.toGenericString() output, analagous to how "modifiers" that also correspond to JVM access flags are handled. This is the initial spec, implementation, and test updated needed for that change. If there is consensus this is a reasonable direction, I'll create the CSR, etc. - Commit messages: - JDK-8322878: Including sealing information Class.toGenericString() Changes: https://git.openjdk.org/jdk/pull/17239/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17239&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322878 Stats: 25 lines in 2 files changed: 16 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17239.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239 PR: https://git.openjdk.org/jdk/pull/17239
