<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>You missed using the variable at l933<br>
</p>
Regards<br>
Prasanta<br>
<div class="moz-cite-prefix">On 12/12/2017 5:21 PM, Krishna
Addepalli wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6c239b79-9901-4b39-a18f-edbc0f43e013@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;}
@font-face
{font-family:"Times New Roman \,serif";
panose-1:0 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 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;
color:black;}
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
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:Consolas;
color:black;}
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;
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-style-priority:99;
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;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle23
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle24
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle25
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle26
{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 Prasanta,<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">Did the change
for caching the result of calling “getRowCount()” into a
variable, as pointed out by you, and here is the new webrev:
</span><a
href="http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kaddepalli/8190281/webrev03/</a><span
style="color:#1F497D"><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">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Krishna</span><span
style="color:#1F497D"><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><span style="color:windowtext">From:</span></b><span
style="color:windowtext"> Prasanta Sadhukhan <br>
<b>Sent:</b> Monday, December 11, 2017 7:24 PM<br>
<b>To:</b> Krishna Addepalli
<a class="moz-txt-link-rfc2396E" href="mailto:krishna.addepalli@oracle.com"><krishna.addepalli@oracle.com></a>;
<a class="moz-txt-link-abbreviated" href="mailto:swing-dev@openjdk.java.net">swing-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: <Swing Dev> [10][JDK-8190281]
Code cleanup in
src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p><span style="font-size:12.0pt"><o:p> </o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 12/11/2017 4:16 PM, Krishna Addepalli
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D">Hi Prasanta,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">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.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">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.</span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">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. <br>
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.<br>
<br>
Regards<br>
Prasanta<br>
<br>
<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Krishna</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:windowtext">From:</span></b><span
style="color:windowtext"> Prasanta Sadhukhan <br>
<b>Sent:</b> Monday, December 11, 2017 4:02 PM<br>
<b>To:</b> Krishna Addepalli <a
href="mailto:krishna.addepalli@oracle.com"
moz-do-not-send="true"><krishna.addepalli@oracle.com></a>;
<a href="mailto:swing-dev@openjdk.java.net"
moz-do-not-send="true">swing-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: <Swing Dev>
[10][JDK-8190281] Code cleanup in
src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p>Hi Krishna,<o:p></o:p></p>
<p class="MsoNormal">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.<br>
<br>
Regards<br>
Prasanta<o:p></o:p></p>
<div>
<p class="MsoNormal">On 12/11/2017 3:01 PM, Krishna
Addepalli wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D">Hi
Prasanta,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks for
pointing out the “getRowCount()==0” check. Modified it
to “getRowCount() <= 0” in the new webrev: </span><a
href="http://cr.openjdk.java.net/%7Ekaddepalli/8190281/webrev02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~kaddepalli/8190281/webrev02/</a><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">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.</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Krishna</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="color:windowtext">From:</span></b><span
style="color:windowtext"> Prasanta Sadhukhan <br>
<b>Sent:</b> Saturday, December 9, 2017 7:54 PM<br>
<b>To:</b> Krishna Addepalli <a
href="mailto:krishna.addepalli@oracle.com"
moz-do-not-send="true"><krishna.addepalli@oracle.com></a>;
<a href="mailto:swing-dev@openjdk.java.net"
moz-do-not-send="true">swing-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: <Swing Dev>
[10][JDK-8190281] Code cleanup in
src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p>Hi Krishna,<o:p></o:p></p>
<p class="MsoNormal">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<o:p></o:p></p>
<div>
<p class="MsoNormal">On 12/7/2017 4:41 PM, Krishna
Addepalli wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="color:#1F497D">Hi
Sergey,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Per our
conversation, I have done the following changes:</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l0 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">1.<span style="font:7.0pt
"Times New Roman""> </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.</span><o:p></o:p></p>
<p class="MsoListParagraph"
style="text-indent:-.25in;mso-list:l0 level1 lfo2"><!--[if !supportLists]--><span
style="mso-list:Ignore">2.<span style="font:7.0pt
"Times New Roman""> </span></span><!--[endif]--><span
style="color:#1F497D">Moved the “++nextIndex” into the
conditional, so that there is no logical change.</span><o:p></o:p></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"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Krishna</span><o:p></o:p></p>
<p class="MsoNormal"><span style="color:#1F497D"> </span><o:p></o:p></p>
<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
href="mailto:swing-dev@openjdk.java.net"
moz-do-not-send="true">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>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New Roman
,serif",serif"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>