Hi Chris,
Latest webrev looks good to me.
Thanks,
Vyom
On Friday 03 August 2018 02:46 PM, Chris Yin wrote:
Hi, Vyom
Thank a lot for your review and comments, inline and update new webrev as below
http://cr.openjdk.java.net/~xyin/8208279/webrev.02/
On 3 Aug 2018, at 3:45 PM, vyom tewari <[email protected]> wrote:
Hi Chris,
Overall looks good to me, couple of minor comment.
1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with specific
exceptions.
Fixed, thanks
2-> In couple of places we are calling "removeFromEnvironment" on context and spec says it will return
"null" if the env is not set. I see that we are setting the required env in "initContext" so it will never be
"null" in our tests, but i suggest you to put just one liner comment that "val" can not be null. This is just
to improve code readability, please feel free to ignore, if you think it is not worth enough.
Sure, added one line comment before each “removeFromEnvironment” call as you
suggested
Thanks,
Chris
Thanks,
Vyom
On Monday 30 July 2018 08:41 AM, Chris Yin wrote:
Please find the new webrev as below, it addressed some similar issues which
mentioned in review comments from another thread RFR 8200151, thanks
webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/
<http://cr.openjdk.java.net/~xyin/8208279/webrev.01/>
Regards,
Chris
On 26 Jul 2018, at 3:37 PM, Chris Yin <[email protected]> wrote:
Please review the changes to add another 8 JNDI tests to
com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks
bug: https://bugs.openjdk.java.net/browse/JDK-8208279
<https://bugs.openjdk.java.net/browse/JDK-8208279>
webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/
<http://cr.openjdk.java.net/~xyin/8208279/webrev.00/>
Regards,
Chris
