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