On 2014-07-21 22:05, Peter Levart wrote:
On 07/21/2014 09:21 PM, Claes Redestad wrote:
Hi,
was asked offline to add a length check at the start (prevents
potentially costly scans for huge, invalid input; negligible
performance impact for normal cases) and realized dash1 < 0 || dash2
< 0 || dash3 < 0 implies dash4 < 0, so the first three checks are
unnecessary and can be skipped:
http://cr.openjdk.java.net/~redestad/8006627/webrev.5
Hi Claes,
dash1 < 0 || dash2 < 0 || dash3 < 0 does not imply dash4 < 0
Take for example the following input: "0-0-0"
dash1 = 1
dash2 = 3
dash3 = -1
dash4 = 1 (again)
Well, this is embarrassing...
but the following check:
if (dash4 < 0 || name.indexOf('-', dash4 + 1) > 0)
is true in this case. It seems either dash4 < 0 or dash5 > 0 for any
number of dashes in the string except exactly 4. It's just that this
is not immediately evident for the casual reader. I recommend adding a
comment for posterity.
... but that explains why all test cases worked and reinforced the
erroneous assumption. Skipping the checks bring back a few percent in my
microbenchmarks, so I guess adding an explanation as to why this
actually works in context in an inline comment is the least we should do
here:
+ // For any valid input, dash1 through dash4 will be positive and dash5
+ // negative, but it's enough to check dash4 and dash5:
+ // - if dash1 is -1, dash4 will be -1
+ // - if dash1 is positive but dash2 is -1, dash4 will be -1
+ // - if dash1 and dash2 is positive, dash3 will be -1, dash4 will be
+ // positive, but so will dash5
http://cr.openjdk.java.net/~redestad/8006627/webrev.6
Thanks!
/Claes
Regards, Peter
/Claes
On 2014-07-21 20:32, Claes Redestad wrote:
Hi,
new webrev which ensures we always throw some kind of IAE for
invalid inputs and adds a few tests to cover this behavior:
http://cr.openjdk.java.net/~redestad/8006627/webrev.4
/Claes
On 2014-07-21 20:05, Claes Redestad wrote:
Hi,
IIOB is invalid to throw here, so I'll fix that.
NumberFormatException is a IllegalArgumentException, so I think
it's a gray area if the dash4 + 1 < name.length()-check is needed.
I sincerely hope keeping error messages as-is isn't required, though.
/Claes
On 2014-07-21 18:51, Peter Levart wrote:
Hi Claes,
Invalid inputs to UUID.fromString() behave a little different than
before. IllegalArgumentException is not thrown for the following
inputs:
For example:
"0": IllegalArgumentException: Invalid UUID string: 0 (before patch)
"0": IndexOutOfBoundsException (after patch)
"-0": IllegalArgumentException: Invalid UUID string: -0 (before
patch)
"-0": NumberFormatException (after patch)
"0-0-0-0-": IllegalArgumentException: Invalid UUID string:
0-0-0-0- (before patch)
"0-0-0-0-": NumberFormatException (after patch)
The following input (and similar) do throw NumberFormatException
as before, but messages are a little different. That's OK, I suppose.
"0-0-0-x-0": NumberFormatException: For input string: "x" (before
patch)
"0-0-0-x-0": NumberFormatException: Error at index 1 in: "x"
(after patch)
"0-0-0--0": NumberFormatException: For input string: "" (before
patch)
"0-0-0--0": NumberFormatException: (after patch)
The 1st 3 examples could be fixed by checking that dash1,2,3,4 are
all > 0 and that dash4 + 1 < name.length()
Regards, Peter
On 07/21/2014 01:41 PM, Claes Redestad wrote:
On 07/19/2014 02:59 PM, Ivan Gerasimov wrote:
This looks just beautiful!
Thanks!
But why do you need the digits() function at all?
In my opinion, using formatUnsignedLong directly would be no
less clearer.
Sure!
http://cr.openjdk.java.net/~redestad/8006627/webrev.2/
Small improvement with client compiler; no measurable change with
tiered.
/Claes
Sincerely yours,
Ivan
On 19.07.2014 8:59, Claes Redestad wrote:
Hi,
after recent changes, this patch has been revisited and
improved slightly, primarily simplifying and speeding up the
toString method slightly more:
http://cr.openjdk.java.net/~redestad/8006627/webrev.1/
/Claes
On 2014-06-15 00:41, Claes Redestad wrote:
Hi,
please review this patch to improve UUID performance,
originally proposed by Steven Schlansker, rebased to use the
allocation-free methods added in
https://bugs.openjdk.java.net/browse/JDK-8041972
Webrev: http://cr.openjdk.java.net/~redestad/8006627/webrev.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8006627
Thanks!
/Claes
