<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Krishna,<br>
</p>
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. <br>
Also, there is no need of calling getRowCount() twice as it will not
change between 929, 936.<br>
<br>
Regards<br>
Prasanta<br>
<div class="moz-cite-prefix">On 12/7/2017 4:41 PM, Krishna Addepalli
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8ff212fc-7c49-4963-99af-477ac4a0f4e1@default">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
span.EmailStyle19
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:931817106;
mso-list-type:hybrid;
mso-list-template-ids:1614576216 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l0:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D">Hi Sergey,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Per our
conversation, I have done the following changes:<o:p></o:p></span></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l0 level1 lfo1"><!--[if !supportLists]--><span
style="color:#1F497D"><span style="mso-list:Ignore">1.<span
style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]--><span
style="color:#1F497D">Found that the .class size increases
by 1kb when streams are used, so reverted the changes
related to it.<o:p></o:p></span></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l0 level1 lfo1"><!--[if !supportLists]--><span
style="color:#1F497D"><span style="mso-list:Ignore">2.<span
style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]--><span
style="color:#1F497D">Moved the “++nextIndex” into the
conditional, so that there is no logical change.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Here is the
updated webrev: </span><a
href="http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kaddepalli/8190281/webrev01/</a><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Krishna<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"
moz-do-not-send="true"><span style="color:#1F497D"><o:p> </o:p></span></a></p>
<span style="mso-bookmark:_MailEndCompose"></span>
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Krishna Addepalli <br>
<b>Sent:</b> Wednesday, December 6, 2017 2:43 PM<br>
<b>To:</b> <a class="moz-txt-link-abbreviated" href="mailto:swing-dev@openjdk.java.net">swing-dev@openjdk.java.net</a><br>
<b>Subject:</b> [10][JDK-8190281] Code cleanup in
src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Hi All,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Please review the fix for bug:<o:p></o:p></p>
<p class="MsoNormal">Bug: JDK-8190281 <a
href="https://bugs.openjdk.java.net/browse/JDK-8190281"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8190281</a><o:p></o:p></p>
<p class="MsoNormal">JDK 10 Webrev: <a
href="http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kaddepalli/8190281/webrev00/</a><o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">This bug was created while root causing
JDK-8187936, and the following refactoring points have been
addressed:<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">1. Line 927: Uninitialized variables,
checking for trivial reject case multiple times. <br>
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. <br>
3. Line 1365: Code repetition for differenct conditions, which
can be ored together to reduce the repetition. <br>
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. <br>
5. Line 1505: Variable initialization can be simplified by
combining different conditions. <br>
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.<br>
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.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
<p class="MsoNormal">Krishna<o:p></o:p></p>
</div>
</blockquote>
<br>
</body>
</html>