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.addepa...@oracle.com>;
swing-dev@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.addepa...@oracle.com>
<mailto:krishna.addepa...@oracle.com>; swing-dev@openjdk.java.net
<mailto:swing-dev@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.addepa...@oracle.com>
<mailto:krishna.addepa...@oracle.com>;
swing-dev@openjdk.java.net <mailto:swing-dev@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.addepa...@oracle.com>
<mailto:krishna.addepa...@oracle.com>;
swing-dev@openjdk.java.net <mailto:swing-dev@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@openjdk.java.net
<mailto:swing-dev@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
