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 >
