Re: ByteArrayOutputStream should not have a new writeBytes method in Java
Hello Stuart, I'm not sure whether you're aware that IntelliJ IDEA has an automatic inspection named "C-style array declaration": - description: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti... - logic: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti... You can run such single inspection over the entire project as described here: https://www.jetbrains.com/help/idea/running-inspection-by-name.html It will fix all such C-style array declarations for you automatically. -- Regards, Tomasz Linkowski From: Stuart Marks <stuart.marks@oracle.com>
To: "ullenboom@gmail.com" <ullenboom@gmail.com> Cc: "core-libs-dev@openjdk.java.net" <core-libs-dev@openjdk.java.net> Bcc: Date: Tue, 11 Sep 2018 13:23:41 -0700 Subject: Re: ByteArrayOutputStream should not have a new writeBytes method in Java 11
2. even if, it should not be byte b[] but byte[] b
Yeah we need to clean occurrences of this anachronistic array declaration from the JDK. I noticed a few others nearby. It's startling that a new occurrence has crept it. (Or maybe not, perhaps it was done to conform to the nearby code.)
Any volunteers to clean this up?
An interesting exercise would be to write a detector for this declaration style.
s'marks
Cool, I didn't know that IntelliJ IDEA has such an inspection, but I'm not surprised. Perhaps a volunteer can run this inspection over parts of the JDK code base and see what comes up, and possibly propose some patches. :-) s'marks On 9/11/18 10:34 PM, Tomasz Linkowski wrote:
Hello Stuart,
I'm not sure whether you're aware that IntelliJ IDEA has an automatic inspection named "C-style array declaration": - description: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti... - logic: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti...
You can run such single inspection over the entire project as described here: https://www.jetbrains.com/help/idea/running-inspection-by-name.html
It will fix all such C-style array declarations for you automatically.
-- Regards, Tomasz Linkowski
From: Stuart Marks <stuart.marks@oracle.com <mailto:stuart.marks@oracle.com>> To: "ullenboom@gmail.com <mailto:ullenboom@gmail.com>" <ullenboom@gmail.com <mailto:ullenboom@gmail.com>> Cc: "core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>" <core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>> Bcc: Date: Tue, 11 Sep 2018 13:23:41 -0700 Subject: Re: ByteArrayOutputStream should not have a new writeBytes method in Java 11
2. even if, it should not be byte b[] but byte[] b
Yeah we need to clean occurrences of this anachronistic array declaration from the JDK. I noticed a few others nearby. It's startling that a new occurrence has crept it. (Or maybe not, perhaps it was done to conform to the nearby code.)
Any volunteers to clean this up?
An interesting exercise would be to write a detector for this declaration style.
s'marks
Hello! I can volunteer doing this if OpenJDK community is really interested. I've launched the inspection over all java.* and jdk.* modules and found 4064 warnings. As an example, I converted all C-style array declarations in java.base module (660 warnings in 168 files). Here's the webrev: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/ To me it's easier to review the whole patch rather than click on every file: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/jdk.patch I checked the result by eyes thoroughly and did not find anything wrong. So if you're interested I can prepare a patch which covers any full JDK sources or any subset of it. The only questions are which subset should be processed and should I also update copyrights for changed files (doing this automatically could be more risky unless there's already well-tested utility to do this). With best regards, Tagir Valeev. On Thu, Sep 13, 2018 at 8:02 AM Stuart Marks <stuart.marks@oracle.com> wrote:
Cool, I didn't know that IntelliJ IDEA has such an inspection, but I'm not surprised.
Perhaps a volunteer can run this inspection over parts of the JDK code base and see what comes up, and possibly propose some patches. :-)
s'marks
On 9/11/18 10:34 PM, Tomasz Linkowski wrote:
Hello Stuart,
I'm not sure whether you're aware that IntelliJ IDEA has an automatic inspection named "C-style array declaration": - description: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti... - logic: https://github.com/JetBrains/intellij-community/blob/master/plugins/Inspecti...
You can run such single inspection over the entire project as described here: https://www.jetbrains.com/help/idea/running-inspection-by-name.html
It will fix all such C-style array declarations for you automatically.
-- Regards, Tomasz Linkowski
From: Stuart Marks <stuart.marks@oracle.com <mailto:stuart.marks@oracle.com>> To: "ullenboom@gmail.com <mailto:ullenboom@gmail.com>" <ullenboom@gmail.com <mailto:ullenboom@gmail.com>> Cc: "core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>" <core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>> Bcc: Date: Tue, 11 Sep 2018 13:23:41 -0700 Subject: Re: ByteArrayOutputStream should not have a new writeBytes method in Java 11
2. even if, it should not be byte b[] but byte[] b
Yeah we need to clean occurrences of this anachronistic array declaration from the JDK. I noticed a few others nearby. It's startling that a new occurrence has crept it. (Or maybe not, perhaps it was done to conform to the nearby code.)
Any volunteers to clean this up?
An interesting exercise would be to write a detector for this declaration style.
s'marks
On 28/09/2018 05:51, Tagir Valeev wrote:
Hello!
I can volunteer doing this if OpenJDK community is really interested. I've launched the inspection over all java.* and jdk.* modules and found 4064 warnings. As an example, I converted all C-style array declarations in java.base module (660 warnings in 168 files). Here's the webrev: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/
To me it's easier to review the whole patch rather than click on every file: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/jdk.patch
I checked the result by eyes thoroughly and did not find anything wrong. So if you're interested I can prepare a patch which covers any full JDK sources or any subset of it. The only questions are which subset should be processed and should I also update copyrights for changed files (doing this automatically could be more risky unless there's already well-tested utility to do this).
Good to see this. Updating the copyright dates is annoying but if you can do that then it would be good. There is a script to do a bulk update that I think is better for cases like this but it doesn't seem to be run very often these days. As regards doing the entire source base then I think that would be good. Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk. -Alan
I can review the changes in java.desktop, please use these email alias awt-dev/2d-dev/swing-dev. On 28/09/2018 04:13, Alan Bateman wrote:
As regards doing the entire source base then I think that would be good. Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk.
-- Best regards, Sergey.
Hello! Ok, let's start with smaller thing which is java.desktop. Created a JBS issue and posted a patch here: http://mail.openjdk.java.net/pipermail/2d-dev/2018-October/009486.html Jonathan,
Although cleanup like this is nice, I'll give a word of warning that pervasive changes like that can sometimes cause difficulties when there are lots of changes in code which is undergoing different lines of development in different branches or repos. If nothing else, I'd consider doing it on a per-component or per-module basis.
I'm not sure I have enough time to post 70+ reviews for every module and track all of them. My volunteering abilities are quite limited :-) I think I can manage up to 3-4 separate changesets including already posted java.desktop change. If you have suggestions on how to split this to several big parts, you are welcome (I don't know how "components" are mapped to modules, probably components are big enough?). Or probably we can cover only part of modules for now and wait for the next volunteer to pick up this. With best regards, Tagir Valeev. On Sat, Sep 29, 2018 at 5:41 AM Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
I can review the changes in java.desktop, please use these email alias awt-dev/2d-dev/swing-dev.
On 28/09/2018 04:13, Alan Bateman wrote:
As regards doing the entire source base then I think that would be good. Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk.
-- Best regards, Sergey.
Thank you very much for working on these cleanups. I've occasionally done similar in the past. I've made big changesets with only one automated change at a time. So e.g. I would do only "C-style array declaration" in one changeset. This is one example of a change that should leave zero impact on the generated bytecode, and that should be testable, if only by comparing the .class files for identical size. One difficulty is that the copy of the source code in openjdk may not be the "master" copy - it may be imported from some other project, and in general it's hard to tell. On Sun, Sep 30, 2018 at 8:40 PM, Tagir Valeev <amaembo@gmail.com> wrote:
Hello!
Ok, let's start with smaller thing which is java.desktop. Created a JBS issue and posted a patch here: http://mail.openjdk.java.net/pipermail/2d-dev/2018-October/009486.html
Jonathan,
Although cleanup like this is nice, I'll give a word of warning that pervasive changes like that can sometimes cause difficulties when there are lots of changes in code which is undergoing different lines of development in different branches or repos. If nothing else, I'd consider doing it on a per-component or per-module basis.
I'm not sure I have enough time to post 70+ reviews for every module and track all of them. My volunteering abilities are quite limited :-) I think I can manage up to 3-4 separate changesets including already posted java.desktop change. If you have suggestions on how to split this to several big parts, you are welcome (I don't know how "components" are mapped to modules, probably components are big enough?). Or probably we can cover only part of modules for now and wait for the next volunteer to pick up this.
With best regards, Tagir Valeev.
On Sat, Sep 29, 2018 at 5:41 AM Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
I can review the changes in java.desktop, please use these email alias awt-dev/2d-dev/swing-dev.
On 28/09/2018 04:13, Alan Bateman wrote:
As regards doing the entire source base then I think that would be
good.
Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk.
-- Best regards, Sergey.
On 10/1/18 10:46 AM, Martin Buchholz wrote:
This is one example of a change that should leave zero impact on the generated bytecode, and that should be testable, if only by comparing the .class files for identical size.
This is a good point. I did something similar when I did the "diamond" conversion way back in JDK 7. That conversion didn't change any bytecodes, I compared class files to ensure the results were identical after the change. That provided greater assurance of correctness for large changesets. s'marks
Hello! So the conversion was successfully performed for client components: https://bugs.openjdk.java.net/browse/JDK-8211300 http://hg.openjdk.java.net/jdk/client/rev/2e330da7cbf4 https://bugs.openjdk.java.net/browse/JDK-8211693 http://hg.openjdk.java.net/jdk/client/rev/de9486d74a74 Now we can move to something else. Let's limit the array declaration conversion to core-libs for now. I created an issue JDK-8211981. For me the problem now is to determine which modules belong to core-libs. I tried to find this information. The only thing I found is this apparently outdated page: http://openjdk.java.net/groups/core-libs/ It lists packages, but not modules, and links to Java 8 spec. Probably it's time to update it? To me it seems that the following modules belong to core-libs: java.base, java.naming, java.rmi, java.scripting, java.sql, java.sql.rowset, java.xml, java.xml.crypto, jdk.unsupported, jdk.xml.dom. Anything missing or added by mistake? With best regards, Tagir Valeev. On Mon, Oct 1, 2018 at 10:40 AM Tagir Valeev <amaembo@gmail.com> wrote:
Hello!
Ok, let's start with smaller thing which is java.desktop. Created a JBS issue and posted a patch here: http://mail.openjdk.java.net/pipermail/2d-dev/2018-October/009486.html
Jonathan,
Although cleanup like this is nice, I'll give a word of warning that pervasive changes like that can sometimes cause difficulties when there are lots of changes in code which is undergoing different lines of development in different branches or repos. If nothing else, I'd consider doing it on a per-component or per-module basis.
I'm not sure I have enough time to post 70+ reviews for every module and track all of them. My volunteering abilities are quite limited :-) I think I can manage up to 3-4 separate changesets including already posted java.desktop change. If you have suggestions on how to split this to several big parts, you are welcome (I don't know how "components" are mapped to modules, probably components are big enough?). Or probably we can cover only part of modules for now and wait for the next volunteer to pick up this.
With best regards, Tagir Valeev.
On Sat, Sep 29, 2018 at 5:41 AM Sergey Bylokhov <Sergey.Bylokhov@oracle.com> wrote:
I can review the changes in java.desktop, please use these email alias awt-dev/2d-dev/swing-dev.
On 28/09/2018 04:13, Alan Bateman wrote:
As regards doing the entire source base then I think that would be good. Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk.
-- Best regards, Sergey.
On 09/28/2018 04:13 AM, Alan Bateman wrote:
On 28/09/2018 05:51, Tagir Valeev wrote:
Hello!
I can volunteer doing this if OpenJDK community is really interested. I've launched the inspection over all java.* and jdk.* modules and found 4064 warnings. As an example, I converted all C-style array declarations in java.base module (660 warnings in 168 files). Here's the webrev: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/
To me it's easier to review the whole patch rather than click on every file: http://cr.openjdk.java.net/~tvaleev/patches/array_decl_java_base/jdk.patch
I checked the result by eyes thoroughly and did not find anything wrong. So if you're interested I can prepare a patch which covers any full JDK sources or any subset of it. The only questions are which subset should be processed and should I also update copyrights for changed files (doing this automatically could be more risky unless there's already well-tested utility to do this).
Good to see this. Updating the copyright dates is annoying but if you can do that then it would be good. There is a script to do a bulk update that I think is better for cases like this but it doesn't seem to be run very often these days.
As regards doing the entire source base then I think that would be good. Due to the complexity of testing, changes to the java.desktop module are pushed to jdk/client repo rather than jdk/jdk so if it's not too awkward then it might be helper if the patch for java.desktop were a separate change that gets pushed to jdk/client rather than jdk/jdk.
-Alan
Although cleanup like this is nice, I'll give a word of warning that pervasive changes like that can sometimes cause difficulties when there are lots of changes in code which is undergoing different lines of development in different branches or repos. If nothing else, I'd consider doing it on a per-component or per-module basis. As regards copyrights, there are scripts to fix dates that only modify the affected files (as compared to what I think Alan is referring to, which is a script to modify all files in the repo which have been edited that year.) -- Jon
participants (7)
-
Alan Bateman
-
Jonathan Gibbons
-
Martin Buchholz
-
Sergey Bylokhov
-
Stuart Marks
-
Tagir Valeev
-
Tomasz Linkowski