[OpenJDK 2D-Dev] RFR JDK-8211300: Convert C-style array declarations in Java code

Wed Oct 3 10:06:18 UTC 2018

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
> >>
> >
> >