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

Tue Dec 12 13:36:43 UTC 2017


On 12/12/2017 7:04 PM, Prasanta Sadhukhan wrote:
>
> You missed using the variable at l933
>
and l955,956
>
> 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/5f51a802/attachment.html>