Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication [v2]
> `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` > +`put` calls. > > https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165 > > Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to > follow. We know that `requests` can contain only non-null values. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication remove obvious assert - Changes: - all: https://git.openjdk.java.net/jdk/pull/8484/files - new: https://git.openjdk.java.net/jdk/pull/8484/files/b493aab1..f850d61f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8484=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8484=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8484/head:pull/8484 PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication [v2]
On Wed, 1 Jun 2022 13:27:27 GMT, Andrey Turbanov wrote: >> src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java >> line 159: >> >>> 157: if (t == null || t == c) { >>> 158: assert cached == null; >>> 159: return cached; >> >> Hello Andrey, while you are in this code, I think changing these 2 lines: >> >> >> assert cached == null; >> return cached; >> >> to just: >> >> >> return null; >> >> would be better. There's already a `if (cached != null) return cached;` >> code, a few lines above and after that line there's no other modifications >> to this `cached` local variable, so changing this line to just return null >> would remove any confusion while reading this code. > > Good idea. Updated. Thank you for that change. The changes in this PR looks fine to me. - PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication [v2]
On Wed, 1 Jun 2022 13:32:28 GMT, Andrey Turbanov wrote: >> `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` >> +`put` calls. >> >> https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165 >> >> Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to >> follow. We know that `requests` can contain only non-null values. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication > remove obvious assert Marked as reviewed by jpai (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication [v2]
On Wed, 1 Jun 2022 04:08:53 GMT, Jaikiran Pai wrote: >> Andrey Turbanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication >> remove obvious assert > > src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java > line 159: > >> 157: if (t == null || t == c) { >> 158: assert cached == null; >> 159: return cached; > > Hello Andrey, while you are in this code, I think changing these 2 lines: > > > assert cached == null; > return cached; > > to just: > > > return null; > > would be better. There's already a `if (cached != null) return cached;` code, > a few lines above and after that line there's no other modifications to this > `cached` local variable, so changing this line to just return null would > remove any confusion while reading this code. Good idea. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
On Sat, 30 Apr 2022 10:17:43 GMT, Andrey Turbanov wrote: > `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` > +`put` calls. > > https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165 > > Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to > follow. We know that `requests` can contain only non-null values. LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8484
Re: RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
On Sat, 30 Apr 2022 10:17:43 GMT, Andrey Turbanov wrote: > `AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` > +`put` calls. > > https://github.com/openjdk/jdk/blob/176bb23de18d9ab448e77e85a9c965a7c02f2c50/src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java#L155-L165 > > Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to > follow. We know that `requests` can contain only non-null values. src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java line 159: > 157: if (t == null || t == c) { > 158: assert cached == null; > 159: return cached; Hello Andrey, while you are in this code, I think changing these 2 lines: assert cached == null; return cached; to just: return null; would be better. There's already a `if (cached != null) return cached;` code, a few lines above and after that line there's no other modifications to this `cached` local variable, so changing this line to just return null would remove any confusion while reading this code. - PR: https://git.openjdk.java.net/jdk/pull/8484
RFR: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
`AuthenticationInfo.requestAuthentication` uses separate `HashMap`'s `get` +`put` calls. Thread t, c; c = Thread.currentThread(); if ((t = requests.get(key)) == null) { requests.put (key, c); assert cached == null; return cached; } if (t == c) { assert cached == null; return cached; } Instead we can use the `HashMap.putIfAbsent` to make code a bit easier to follow. We know that `requests` can contain only non-null values. - Commit messages: - [PATCH] Cleanup Map usage in AuthenticationInfo.requestAuthentication Changes: https://git.openjdk.java.net/jdk/pull/8484/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8484=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287390 Stats: 10 lines in 1 file changed: 0 ins; 5 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8484/head:pull/8484 PR: https://git.openjdk.java.net/jdk/pull/8484