Hello! > Given what Sergey has done, I don't need to repeat that work. > I am now just looking for formatting anomalies, but there is a lot to > look at. > I should be able to OK this tomorrow and we can then commit it on your > behalf.
Thanks, that would be great! > > Have you looked at the other client modules ? datatransfer, accessibility, > jdk.unsupported.desktop, or the client demos ? No, I haven't. I was not aware that there are other client modules. I assume that client demos are in src/demo/share directory, right? I found no hits in datatransfer and jdk.unsupported.desktop, 4 files in accessibility and 71 files in demos. Here's separate webrev for these 75 files: http://cr.openjdk.java.net/~tvaleev/patches/c_style_arrays_demos_accessibility/ Should I post a separate JBS issue for this (or two issues) or it could be incorporated within existing issue? Should I post separate reviews for accessibility and demos or it's fine to have them together? > If you are crazy (just an idea) then maybe even tests ! > But that latter one is a low priority. Let's postpone tests. I'd like to move to some core modules like java.base until my volunteering energy runs out :-) > > It appeared that compound declarations like `byte r[], g[], b[];` > > are converted to `byte[] r;byte[] g; byte[] b; > > Seems like someone should fix their tool :-) Yeah, this is what I'm thinking about (I'm IDEA developer). See, IDEA processes every hit separately, and we have three independent hits here. So when it processes r[] it doesn't know that I want to convert others as well, thus it has to split the compound declaration into `byte[] r;byte g[], b[];` (when formatting is active, they are placed on separate lines, but I switched off the formatting to simplify the changeset). The same is repeated for `g` and `b` independently and we have unpleasant result. Probably we can fix this issuing single warning per declaration and suggesting to fix the whole declaration or nothing (it's unlikely that somebody would like to fix only some of these arrays), but then the question is which source element should be highlighted. Now we highlight three pairs of brackets in three individual warnings, but if we have a single warning, we should highlight something continuous. We may highlight the whole declaration, but what if it also contains non-convertible things like `byte r[], g, b[]`. Looks like a simple problem, but I don't see the trivial solution. But ok, it's an off-topic :-) With best regards, Tagir Valeev > > -phil. > > > On 10/1/18, 6:24 PM, Sergey Bylokhov wrote: > > Looks fine to me. > > I have checked the patch, have build the change and run some tests on > > our supported platforms. > > > > On 30/09/2018 20:29, Tagir Valeev wrote: > >> Hello! > >> > >> Please review JDK-8211300 [1] this change: [2]. Also it needs a > >> sponsor as I have only JDK author status in OpenJDK Census [3]. > >> > >> The discussion in core-libs-dev [4] shows that it's desired to get rid > >> of old style array declarations like `int array[]` and massively > >> convert them to `int[] array`. I volunteered to work on this. As Alan > >> Bateman noted [5], java.desktop module is pushed to separate repo, so > >> it would be better to process it separately, thus I'd like to start > >> with it. > >> > >> The changeset was created in the following steps: > >> * Import JDK sources to IntelliJ IDEA > >> * Mark java.desktop/aix/classes, java.desktop/macosx/classes, > >> java.desktop/unix/classes, java.desktop/solaris/classes, > >> java.desktop/windows/classes and java.desktop/share/classes as source > >> roots > >> * Disable automatic formatting on the whole project > >> * Run the inspection "C-style array declaration" > >> * Apply the quick-fix massively > >> * Perform regex replace over changed files only: > >> Search: Copyright \(c\) (\d+), (\d+), Oracle and/or its affiliates. > >> All rights reserved. > >> Replace: Copyright \(c\) $1, 2018, Oracle and/or its affiliates. All > >> rights reserved. > >> * Perform regex replace over changed files only: > >> Search: Copyright \(c\) (\d+), Oracle and/or its affiliates. All > >> rights reserved. > >> Replace: Copyright \(c\) $1, 2018, Oracle and/or its affiliates. All > >> rights reserved. > >> * It appeared that compound declarations like `byte r[], g[], b[];` > >> are converted to `byte[] r;byte[] g; byte[] b;` which does not look > >> nice while compilable. I found three such cases via simple regexp > >> search in BMPImageReader#658, BMPImageWriter#325 and > >> AreaAveragingScaleFilter#66 and fixed them manually. > >> > >> In total 339 files were changed with 1335 lines of array declaration > >> updates and 310 lines of copyright updates. I reviewed the generated > >> patch by eyes, but only partially, because it's too big. Also I > >> performed some kind of simple sanity check using regexps: > >> > >> $ grep '^+[^+]' jdk.patch | grep -v 'Oracle and/or its affiliates. All > >> rights reserved'|grep -v '\[\]'|wc > >> 0 0 0 > >> (check that every updated line contains either copyright message or []) > >> > >> With best regards, > >> Tagir Valeev. > >> > >> [1] https://bugs.openjdk.java.net/browse/JDK-8211300 > >> [2] http://cr.openjdk.java.net/~tvaleev/webrev/8211300/r1/ > >> [3] http://openjdk.java.net/census#tvaleev > >> [4] > >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055390.html > >> [5] > >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055759.html > >> > > > >
