<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
Tue Dec 12 14:38:20 UTC 2017
As told, you overlooked l955,956
Regards
Prasanta
On 12/12/2017 7:37 PM, Krishna Addepalli wrote:
>
> Oops! My bad. Created a new webrev here with the correction:
> http://cr.openjdk.java.net/~kaddepalli/8190281/webrev04/
> <http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev04/>
>
> *From:*Prasanta Sadhukhan
> *Sent:* Tuesday, December 12, 2017 7:05 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
>
> 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>
> <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
>
> 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/e2090bfa/attachment.html>
More information about the swing-dev
mailing list