Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 13:49:56 GMT, Pavel Rappo wrote: >> yes, >> javac should emit a warning in that case, that also the answer of Brian. >> >> https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html > > Then we should use an unbounded wildcard here and in similar places to avoid > "rawtype" warnings in the future. I've added the wildcard to the pattern variable check now. See b89027c - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Tue, 16 Mar 2021 19:26:35 GMT, Daniel Fuchs wrote: >> `declaringClass` is a string representing the class name (not the `Class` >> object). The variable name indeed causes confusion. > > Doh. My mistake. Was thinking about `StackFrame`. Thanks Mandy! I've reordered the checks as suggested and the updated code can be found in commit b89027c - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Tue, 16 Mar 2021 17:24:56 GMT, Mandy Chung wrote: >> Hi Rémi, >> There seems to be a deeper issue here - Patrick pointed that out to me - the >> specification of equals above speaks of comparing class names where the >> actual implementation compares classes. Maybe the specification should be >> updated - but that would be better done in a separate issue with CSR etc... >> Maybe we can do the optimization you suggest at the same time? > > `declaringClass` is a string representing the class name (not the `Class` > object). The variable name indeed causes confusion. Doh. My mistake. Was thinking about `StackFrame`. Thanks Mandy! - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Tue, 16 Mar 2021 11:13:50 GMT, Daniel Fuchs wrote: >> src/java.base/share/classes/java/lang/StackTraceElement.java line 413: >> >>> 411: && Objects.equals(moduleName, e.moduleName) >>> 412: && Objects.equals(moduleVersion, e.moduleVersion) >>> 413: && e.declaringClass.equals(declaringClass) >> >> testing the declaring class and the line number should be done first given >> they are primitive values, it will be a little more efficient if two >> StackTraceElement are not equals and one is using non interned String. >> return (obj instanceof StackTraceElement e) >> && e.lineNumber == lineNumber >> && e.declaringClass == declaringClass >> && ... > > Hi Rémi, > There seems to be a deeper issue here - Patrick pointed that out to me - the > specification of equals above speaks of comparing class names where the > actual implementation compares classes. Maybe the specification should be > updated - but that would be better done in a separate issue with CSR etc... > Maybe we can do the optimization you suggest at the same time? `declaringClass` is a string representing the class name (not the `Class` object). The variable name indeed causes confusion. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:35:27 GMT, Rémi Forax wrote: >> Patrick Concannon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263358: Refactored missed equals method > > src/java.base/share/classes/java/lang/StackTraceElement.java line 413: > >> 411: && Objects.equals(moduleName, e.moduleName) >> 412: && Objects.equals(moduleVersion, e.moduleVersion) >> 413: && e.declaringClass.equals(declaringClass) > > testing the declaring class and the line number should be done first given > they are primitive values, it will be a little more efficient if two > StackTraceElement are not equals and one is using non interned String. > return (obj instanceof StackTraceElement e) > && e.lineNumber == lineNumber > && e.declaringClass == declaringClass > && ... Hi Rémi, There seems to be a deeper issue here - Patrick pointed that out to me - the specification of equals above speaks of comparing class names where the actual implementation compares classes. Maybe the specification should be updated - but that would be better done in a separate issue with CSR etc... Maybe we can do the optimization you suggest at the same time? - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 13:34:45 GMT, Rémi Forax wrote: >> I don't think that cast from `Object` to a raw type is unchecked, so as >> error does not seem warranted to me. >> >> However, I agree javac should produce the rawtype warning for rawtypes in >> pattern matching instanceof, because we are introducing a new variable (and >> casting). I've filled: >> https://bugs.openjdk.java.net/browse/JDK-8263590 >> >> Note the non-pattern matching instanceof does not produce the rawtype >> warning, and I don't think it should produce it. > > yes, > javac should emit a warning in that case, that also the answer of Brian. > > https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html Then we should use an unbounded wildcard here and in similar places to avoid "rawtype" warnings in the future. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:39:20 GMT, Jan Lahoda wrote: >> We have considered generics, that why parameterized generics with another >> type arguments are forbidden, but i think we have forgotten raw types. > > I don't think that cast from `Object` to a raw type is unchecked, so as error > does not seem warranted to me. > > However, I agree javac should produce the rawtype warning for rawtypes in > pattern matching instanceof, because we are introducing a new variable (and > casting). I've filled: > https://bugs.openjdk.java.net/browse/JDK-8263590 > > Note the non-pattern matching instanceof does not produce the rawtype > warning, and I don't think it should produce it. yes, javac should emit a warning in that case, that also the answer of Brian. https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:09:45 GMT, Rémi Forax wrote: >>> I think it's a bug in javac, it should not even raise a warning but an >>> error. >>> But the spec is not fully clear. >> >> If you think that it's a bug, consider providing feedback to authors of JEP >> 394: >> >>> Other refinements may be incorporated based on further feedback. >> >> I find it hard to believe that authors didn't consider generic use case >> (even though JEP 394 doesn't have examples of that). > > We have considered generics, that why parameterized generics with another > type arguments are forbidden, but i think we have forgotten raw types. I don't think that cast from `Object` to a raw type is unchecked, so as error does not seem warranted to me. However, I agree javac should produce the rawtype warning for rawtypes in pattern matching instanceof, because we are introducing a new variable (and casting). I've filled: https://bugs.openjdk.java.net/browse/JDK-8263590 Note the non-pattern matching instanceof does not produce the rawtype warning, and I don't think it should produce it. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:01:48 GMT, Pavel Rappo wrote: >> The problem is that `desc` is a raw type and raw types doesn't play well >> with the rest of the language (in particular with inference). >> >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. >> >> The spec says that the narrow conversion should not be unchecked >> https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2 >> it depends if you consider the raw conversion as unchecked or not. > >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. > > If you think that it's a bug, consider providing feedback to authors of JEP > 394: > >> Other refinements may be incorporated based on further feedback. > > I find it hard to believe that authors didn't consider generic use case (even > though JEP 394 doesn't have examples of that). We have considered generics, that why parameterized generics with another type arguments are forbidden, but i think we have forgotten raw types. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 10:47:10 GMT, Rémi Forax wrote: > I think it's a bug in javac, it should not even raise a warning but an error. > But the spec is not fully clear. If you think that it's a bug, consider providing feedback to authors of JEP 394: > Other refinements may be incorporated based on further feedback. I find it hard to believe that authors didn't consider generic use case (even though JEP 394 doesn't have examples of that). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 10:18:26 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
>> 360:
>>
>>> 358: public final boolean equals(Object o) {
>>> 359: if (this == o) return true;
>>> 360: return (o instanceof DynamicConstantDesc desc)
>>
>> should be
>> `(o instanceof DynamicConstantDesc desc)`
>
> I noticed that too initially. However, `javac` does not seem to produce a
> "rawtypes" warning. Which makes sense, if you think about it.
The problem is that `desc` is a raw type and raw types doesn't play well with
the rest of the language (in particular with inference).
I think it's a bug in javac, it should not even raise a warning but an error.
But the spec is not fully clear.
The spec says that the narrow conversion should not be unchecked
https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2
it depends if you consider the raw conversion as unchecked or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:32:33 GMT, Rémi Forax
wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8263358: Refactored missed equals method
>
> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
> 360:
>
>> 358: public final boolean equals(Object o) {
>> 359: if (this == o) return true;
>> 360: return (o instanceof DynamicConstantDesc desc)
>
> should be
> `(o instanceof DynamicConstantDesc desc)`
I noticed that too initially. However, `javac` does not seem to produce a
"rawtypes" warning. Which makes sense, if you think about it.
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method src/java.base/share/classes/java/lang/StackTraceElement.java line 413: > 411: && Objects.equals(moduleName, e.moduleName) > 412: && Objects.equals(moduleVersion, e.moduleVersion) > 413: && e.declaringClass.equals(declaringClass) testing the declaring class and the line number should be done first given they are primitive values, it will be a little more efficient if two StackTraceElement are not equals and one is using non interned String. return (obj instanceof StackTraceElement e) && e.lineNumber == lineNumber && e.declaringClass == declaringClass && ... - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.lang`
>> package to make use of the `instanceof` pattern variable?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8263358: Refactored missed equals method
src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
360:
> 358: public final boolean equals(Object o) {
> 359: if (this == o) return true;
> 360: return (o instanceof DynamicConstantDesc desc)
should be
`(o instanceof DynamicConstantDesc desc)`
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8263358: Refactored missed equals method - Changes: - all: https://git.openjdk.java.net/jdk/pull/2913/files - new: https://git.openjdk.java.net/jdk/pull/2913/files/071fe1eb..f7924d27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2913&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2913&range=03-04 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2913.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2913/head:pull/2913 PR: https://git.openjdk.java.net/jdk/pull/2913
