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>;
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
