<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 01:20:11 UTC 2017
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>;
> 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/20171218/b5dd6ddb/attachment.html>
More information about the swing-dev
mailing list