[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR JDK-8211300: Convert C-style array declarations in Java code
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2018-10-03 23:09:49
Message-ID: f30ec8e6-9dde-4083-b8ab-4fb1bead650f () oracle ! com
[Download RAW message or body]

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.


[prev in list] [next in list] [prev in thread] [next in thread]