Hi Pavel,
thanks for review, even i did not understand why test case is comparing
the hashcode with the hard coded values that's why at first place i did
not removed it. As you mentioned value doesn't matter i will modify
the hashcode test and will send the modified webrev again. I was not
expecting that hashcodetest is testing NPE by comparing the
*"hardcoded"* hash values.
Thanks,
Vyom
On Monday 20 June 2016 08:17 PM, Pavel Rappo wrote:
Vyom,
You've noticed the bug in the code I proposed: "b =" instead of "b +=". Good!
(Sorry about that.)
When I asked about the strings that were commented out, I meant we'd better do
something about it. First of all, because it's almost always not a good idea to
*push a commented out code* without yet another comment explaining why we're
doing so. Otherwise it confuses people and wastes their precious attention
later.
Secondly, commented out or removed, it doesn't really matter in this particular
case. As I understand correctly these lines were there to test
8027570: NullPointerException in URLPermission.hashCode()
If we remove these lines, this issue will no longer be tested. What was the
problem with these hashCodes? Did they become different after the fix? If so,
let's address it. As I understand the original issue, values do not matter, what
matters is that there's no NPE thrown in these cases.
Thanks,
-Pavel
On 20 Jun 2016, at 14:51, Vyom Tewari <vyom.tew...@oracle.com> wrote:
Hi,
Please find the updated webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.1/index.html
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.1/index.html>). I incorporated the
review comments and remove the "HashCodeTest" where it compares the hashcode with
hard coded hashcode.
Thanks,
Vyom
On Monday 20 June 2016 03:57 PM, Pavel Rappo wrote:
Hi Vyom, thanks for looking into this!
0. * Copyright (c) 2013,2016 Oracle and/or its affiliates. All rights reserved.
If I understand correctly, we're not required to update years in the header
manually. But if you decided to do so, please follow the established pattern (in
both files you've changed):
* Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights
reserved.
^ ^
1. I wonder if we could use String convenience methods instead of bringing
dependency on java.util.stream.Collector. Could something like this be a bit
better both in terms of dependencies and readability?
private String actions() {
String b = String.join(",", methods);
if (!requestHeaders.isEmpty()) {
b = ":" + String.join(",", requestHeaders);
}
return b;
}
2. Is there a particular reason for these parentheses?
150 return (expectedActions.equals(urlp.getActions()));
^ ^
3. Why are these lines commented out?
259 static Test[] hashTests = {
260 //hashtest("http://www.foo.com/path";, "GET:X-Foo", 388644203),
261 //hashtest("http:*", "*:*", 3255810)
262 };
IMO they either have to be updates or removed altogether.
Thanks,
-Pavel
On 20 Jun 2016, at 08:44, Vyom Tewari <vyom.tew...@oracle.com> wrote:
ping!
On Saturday 11 June 2016 10:34 AM, vyom wrote:
Hi All,
Please review the below fix.
Bug : JDK-8114860 Behavior of java.net.URLPermission.getActions()
contradicts spec
Webrev : http://cr.openjdk.java.net/~vtewari/8114860/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.0/index.html>
Thanks,
Vyom
