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

Wed Oct 3 23:09:49 UTC 2018

On 03/10/2018 16:03, Phil Race wrote:
> http://cr.openjdk.java.net/~tvaleev/webrev/8211300/r1/src/java.desktop/share/classes/javax/swing/tree/FixedHeightLayoutCache.java.udiff.html 
> 
> http://cr.openjdk.java.net/~tvaleev/webrev/8211300/r1/src/java.desktop/share/classes/javax/swing/tree/VariableHeightLayoutCache.java.udiff.html 
> 
> 
> Sergey, are you going to commit this, or am I ?

I can push it.

> 
> The changes in demos + accessibility are best handled under a different 
> bug id since
> this one is quite far along ..
> 
> -phil.
> 
> On 10/03/2018 03:06 AM, Tagir Valeev wrote:
>> 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 
>>>>>
>>>>>
>>>>
> 

-- 
Best regards, Sergey.