Thanks a lot for your comments David.

On 1/8/13 6:39 PM, David Schlosnagle wrote:
Hi Kurchi,

I had a couple quick comments:

On line 1756, you might want to allocate the StringBuilder normalizedS
= new StringBuilder(s.length()); to avoid resizing during the
appending.
- I am now wondering if I should directly calculate the hashCode instead of
using a StringBuilder.


On line 1754 and 1759, is it worth restructuring the logic to avoid
performing indexOf twice to find the first '%'? This might be
premature optimization as hopefully the URIs are short and that
indexOf would be replaced with an intrinsic if hot enough.
It is possible to store the value of first occurrence of '%' in a variable, but I was
trying to keep the case with no '%' clutter-free.

- Kurchi


Thanks,
Dave

On Tue, Jan 8, 2013 at 8:19 PM, Kurchi Hazra
<kurchi.subhra.ha...@oracle.com>  wrote:
Hi,

     According to RFC 3986[1], hexadecimal digits encoded by a '%' should be
case-insensitive, for example,%A2 and %a2 should be
considered equal. Although, URI.equals() does take this into consideration,
the implementation of URI.hashCode() does not and
returns different hashcodes for two URIs that are similar in all respects
except for the case of the percent-encoded hexadecimal digits.
This fix attempts to construct a normalized string from the string
representing a component before calculating its hashCode.
I converted to upper case for the normalization(and not lower case) as
required by [1].

     For testing the fix, I added an additional test scenario to an existing
test (jdk/test/java/net/URI/Test.java). While I was there, I also made
minor changes to the test so that it does not produce rawtype and other lint
warnings.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171415
Webrev: http://cr.openjdk.java.net/~khazra/7171415/webrev.00/

URI.compareTo() still suffers from the same problem - I am not sure if it
should be dealt with as a separate bug.

Thanks,
Kurchi

[1] http://tools.ietf.org/html/rfc3986#section-6.2.2.1

Reply via email to