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

[Prasanta Sadhukhan]

You missed using the variable at l933

Regards
Prasanta
On 12/12/2017 5:21 PM, Krishna Addepalli wrote:
>
> Hi Prasanta,
>
> Did the change for caching the result of calling “getRowCount()” into 
> a variable, as pointed out by you, and here is the new webrev: 
> http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/ 
> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev03/>
>
> Thanks,
>
> Krishna
>
> *From:*Prasanta Sadhukhan
> *Sent:* Monday, December 11, 2017 7:24 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
>
> On 12/11/2017 4:16 PM, Krishna Addepalli wrote:
>
>     Hi Prasanta,
>
>     Yes, you are right, but as I mentioned earlier, that would need to
>     make one variable declaration for caching before trivial reject
>     case, which I wanted to avoid.
>
>     As for the body of getRowCount() it is just returning
>     “visibleNodes.size()”, which shouldn’t be a (performance)problem
>     if called 2 times as I understand.
>
> But, the whole premise of changing getRowCount() <=0  was that it can 
> be overridden and return -ve. Left to present implementation, we would 
> not have needed "less than" check.
> So, if we are changing one case because of the above reason, then we 
> cannot forego the 2nd case's problem, as it can have any implementation.
>
> Regards
> Prasanta
>
>     Thanks,
>
>     Krishna
>
>     *From:*Prasanta Sadhukhan
>     *Sent:* Monday, December 11, 2017 4:02 PM
>     *To:* Krishna Addepalli <krishna.addepalli at oracle.com>
>     <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
>     <mailto: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,
>
>     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>
>         <mailto:krishna.addepalli at oracle.com>;
>         swing-dev at openjdk.java.net <mailto: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/20171212/052d22e0/attachment.html>