fixed issue #131 by improving the Parse function in uuid.go + unit tests #132
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
I am sorry, but code does not address issue #131 which only references the case when 38 (36+2) bytes are passed in.
This code, if it worked, would also have an adverse impact on the speed of Parse. The speed of parsing is very important to some users of this package.
| @@ -66,6 +66,10 @@ func Parse(s string) (UUID, error) { | |||
| switch len(s) { | |||
| // xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx | |||
| case 36: | |||
| // Ensure proper dash placement | |||
There was a problem hiding this comment.
I do not see any benefit in these changes. Checking for "-" is already done on lines 94-89.
| for i := range uuid { | ||
| uuid[i], ok = xtob(s[i*2], s[i*2+1]) | ||
| if !ok { | ||
| uuid[i] = s[i] |
There was a problem hiding this comment.
This code ignores half of the UUID and will provide the wrong answer.
It is possible that the existing for loop could be rewritten as
for i := 0; i < 32; i += 2 {
uuid[i>>1], ok = xtob(s[i], s[i+1])
...
| for i, x := range [16]int{ | ||
| 0, 2, 4, 6, | ||
| 9, 11, | ||
| 14, 16, | ||
| 19, 21, | ||
| 24, 26, 28, 30, 32, 34, | ||
| } { | ||
| v, ok := xtob(s[x], s[x+1]) | ||
| if !ok { | ||
| if !isValidHexChar(s[x]) || !isValidHexChar(s[x+1]) { |
There was a problem hiding this comment.
This block is both slower and produces the wrong results. both s[x] and s[x+1] are required to determine the value of uuid byte i. The xtob function, which handles this properly is very optimized.
|
Okay, let me remove the PR. Thanks. |
I attempted to solve the most recently opened error here, but it's sensitive as other functions and test functions rely on the original implementation.
If my improvement on the
Parse()function is correct, then bothTestNullUUIDMarshalJSONandTestJSONUnmarshalneeds to either be rewritten or fed with new data.Also, two test data failed, as I mentioned.