Hi Semyon,
I have run the tests under JTree and made sure that the same number of
tests passed before and after my changes.
Thank you for pointing out additional places to clean up, I have done
some additional changes:
1.Removed the variables as pointed out.
2.Changed the traditional for loop into a range based loop.
3.Initialize the variables at the point of declaration in lines 412,
413, 459,460 and 468.
Here is the updated webrev:
http://cr.openjdk.java.net/~kaddepalli/8190281/webrev06/
<http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev06/>
Thanks,
Krishna
*From:*Semyon Sadetsky
*Sent:* Tuesday, December 19, 2017 6:50 AM
*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
Hi Krishna,
Which tests did you run to ensure that the functionality was not affected?
Can you also remove variables changedParent (478), newNode(479) and
oldRow (646) since they are unused.
--Semyon
On 12/13/2017 12:42 AM, Krishna Addepalli wrote:
Hi Prasanta,
Here is the webrev with suggested changes:
http://cr.openjdk.java.net/~kaddepalli/8190281/webrev05/
<http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev05/>
Thanks,
Krishna
*From:*Prasanta Sadhukhan
*Sent:* Wednesday, December 13, 2017 10:42 AM
*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
But there is no compulsion that we need to store getRowCount() in
"max". You can store in some other variable and then "max" point
to that in the loop.
Regards
Prasanta
On 12/12/2017 9:51 PM, Krishna Addepalli wrote:
Hi Prasanta,
The getRowCount() calls l955,956 cannot be removed, since max
variable is getting modified in the while loop at l945. There
is no guarantee that max will still hold the getRowCount()
after the loop exits. So, those calls cannot be removed.
Thanks,
Krishna
*From:*Prasanta Sadhukhan
*Sent:* Tuesday, December 12, 2017 8:08 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
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.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
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
