<Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java

Prasanta Sadhukhan prasanta.sadhukhan at oracle.com
Mon Dec 11 10:32:14 UTC 2017


Hi Krishna,

My point was we can call getRowCount() once at first and store the 
result and use it subsequently. There was no need to call it 2-3 times.

Regards
Prasanta
On 12/11/2017 3:01 PM, Krishna Addepalli wrote:
>
> Hi Prasanta,
>
> Thanks for pointing out the “getRowCount()==0” check. Modified it to 
> “getRowCount() <= 0” in the new webrev: 
> http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/ 
> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev02/>
>
> As for calling the method twice, you are right that we don’t need to 
> call it twice, but in the interest of having trivial reject case 
> first, and then start the variable declarations, had to let be there 
> to be called twice. Precisely for the reason you stated, it shouldn’t 
> matter if we called it twice.
>
> Thanks,
>
> Krishna
>
> *From:*Prasanta Sadhukhan
> *Sent:* Saturday, December 9, 2017 7:54 PM
> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>; 
> swing-dev at openjdk.java.net
> *Subject:* Re: <Swing Dev> [10][JDK-8190281] Code cleanup in 
> src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java
>
> Hi Krishna,
>
> This seems good to me except one thing. You are checking getRowCount() 
> == 0 but there is a chance of test extending VariableHeightLayoutCache 
> and overriding getRowCount to return -ve also as it is an int. In that 
> case, I guess this function will not return -1 which spec mandates [If 
> there are no rows, -1 is returned] so I guess we should check for <=0.
> Also, there is no need of calling getRowCount() twice as it will not 
> change between 929, 936.
>
> Regards
> Prasanta
>
> On 12/7/2017 4:41 PM, Krishna Addepalli wrote:
>
>     Hi Sergey,
>
>     Per our conversation, I have done the following changes:
>
>     1.Found that the .class size increases by 1kb when streams are
>     used, so reverted the changes related to it.
>
>     2.Moved the “++nextIndex” into the conditional, so that there is
>     no logical change.
>
>     Here is the updated webrev:
>     http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/
>     <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev01/>
>
>     Thanks,
>
>     Krishna
>
>     *From:* Krishna Addepalli
>     *Sent:* Wednesday, December 6, 2017 2:43 PM
>     *To:* swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>     *Subject:* [10][JDK-8190281] Code cleanup in
>     src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java
>
>     Hi All,
>
>     Please review the fix for bug:
>
>     Bug: JDK-8190281 https://bugs.openjdk.java.net/browse/JDK-8190281
>
>     JDK 10 Webrev:
>     http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/
>     <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev00/>
>
>     This bug was created while root causing JDK-8187936, and the
>     following refactoring points have been addressed:
>
>     1. Line 927: Uninitialized variables, checking for trivial reject
>     case multiple times.
>     2. Line 999: Traditional code written to find maximum size of
>     components, which can be done without any local variables and
>     explicit looping by replacing with streams.
>     3. Line 1365: Code repetition for differenct conditions, which can
>     be ored together to reduce the repetition.
>     4. Line 1482: A large code block gets repeated only because of
>     different values need to be passed in one line. This can be moved
>     to a variable initialization, and the repeating code blocks can be
>     reduced to one.
>     5. Line 1505: Variable initialization can be simplified by
>     combining different conditions.
>     6. Line 1540: An explicit loop to apply a function over a
>     collection, can be achieved in one line by a forEach construct.  –
>     This is producing some visual artifacts, so ignored.
>     7. Line 1747: Combine all the trivial reject cases into one
>     condition, and also, a potential bug which increments the
>     "nextIndex" value beyond the length of the containing elements.
>     The increment should happen only if the trivial reject case fails.
>
>     Thanks,
>
>     Krishna
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20171211/7a4a061d/attachment.html>


More information about the swing-dev mailing list