Fix boost trac 8630, 9343, 11482, 11483, 12253 improving invalid string generator handling in uuid #17
Conversation
|
I would like to see tests for all 4 forms when generating from a string and there is unexpected extra data at the end of the string. Your test is good for 1 of the 4 different forms. |
jeking3
changed the title
jeking3
changed the title
jeking3
changed the title
|
@eldiener I updated the pull request with the additional tests. If I can be of any assistance in grooming the boost trac backlog, let me know. |
| if (has_dashes) { | ||
| if (i == 6 || i == 8 || i == 10) { | ||
| // if there are dashes, they must be in every slot | ||
| if (i == 6 || i == 8 || i == 10) { |
There was a problem hiding this comment.
Can this not be:
else if (i == 6 || i == 8 || i == 10) {
There was a problem hiding this comment.
It can, I will push a change for it.
| @@ -122,23 +133,27 @@ struct string_generator { | |||
| static char const*const digits_end = digits_begin + 22; | |||
There was a problem hiding this comment.
This isn't your code but can't be take the sizeof(digits_begin) and use that instead of hardcoding 22 and further on down hardcoding '> 21' ? Hardcoding numbers is generally poor c++ if there is a better way to get the hardcoded value from the compiler.
There was a problem hiding this comment.
sizeof(digits_begin) if changed to static wchar_t const digits_begin[] = ... is 46, confirmed by a BOOST_STATIC_ASSERT, so in this case I think the hardcoded values is appropriate. Considering the enhanced testing, any changes here will be caught.
There was a problem hiding this comment.
In the get_value(char c) case it should be:
sizeof(digits_begin)/sizeof(char)
and in the get_value(wchar_t c) case it should be:
sizeof(digits_begin)/sizeof(wchar_t)
so that hardcoded numbers are not used.
There was a problem hiding this comment.
Okay, we'll have to subtract one, as the digits_end needs to point at the trailing NULL, but I will make this change.
|
The code works. Please see my comments for some possible individual changes. I think the docs should be updated to both explain all the valid string formats uuid accepts and specifies that uuid throws a particular exception if the string is not a valid format or contains invalid hex digits. |
|
I wonder if a boost::regex would be sufficient not only to describe the valid format but also enforce and to parse it, in the future. That would force a dependency that folks may not want however, and it may not be documentation-friendly. |
|
I pushed an update that addresses all concerns thus far. |
…put following successful conversion of a uuid in the input string 11483: uuid string generator now throws std::invalid_argument instead of std::runtime_error on an invalid uuid string 12253: make the uuid parser reject invalid data and added negative test cases to prove it
For example, "12b672ea-388d-47da-9c26-bec5bc8a06cb-foo" was not considered invalid before, but it is now.