<Swing Dev> [10][JDK-8190281] Code cleanup in src\java.desktop\share\classes\javax\swing\tree\VariableHeightLayoutCache.java
Semyon Sadetsky
semyon.sadetsky at oracle.com
Tue Dec 19 16:32:28 UTC 2017
Looks good to me.
--Semyon
On 12/19/2017 12:46 AM, Krishna Addepalli wrote:
>
> 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.addepalli at oracle.com>;
> swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
> <mailto:swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net <mailto:swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net <mailto:swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net
> <mailto:swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net
> <mailto:swing-dev at 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.addepalli at oracle.com>
> <mailto:krishna.addepalli at oracle.com>;
> swing-dev at openjdk.java.net
> <mailto:swing-dev at 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 at openjdk.java.net
> <mailto:swing-dev at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20171219/ef31d600/attachment.html>
More information about the swing-dev
mailing list