added string check #142
added string check #142
Conversation
added string check
There was a problem hiding this comment.
It seems that thre original code doesn't call is_valid_length(), 'is_valid_contents()(removed by the PR), andvalidate_contents()(introduced by the PR). We need to add them. I think that it is better to merge the PR before those addition. However,validate_contents()` is pretty complicated due to UTF8 spec. So unit test is required.
Could you add tests?
I think that MQTT_USE_STR_CHECK could be defined in the test code.
include/mqtt/utf8encoded_strings.hpp
Outdated
| validate_contents(std::string const& str) { | ||
| // This code is based on https://www.cl.cam.ac.uk/~mgk25/ucs/utf8_check.c | ||
| auto result = validation::well_formed; | ||
| #if !defined(MQTT_USE_STR_CK) |
There was a problem hiding this comment.
I think that CK is short form of CHECK. I considered CK, CHK, and CHECK. CHECK is better here.
Could you replace it?
include/mqtt/utf8encoded_strings.hpp
Outdated
| auto result = validation::well_formed; | ||
| #if !defined(MQTT_USE_STR_CK) | ||
| static_cast<void>(str); | ||
| #else |
There was a problem hiding this comment.
If #else exists, #if condition should be positive expression.
See https://github.com/redboltz/mqtt_cpp/blob/master/include/mqtt/endpoint.hpp#L4077
In addition, could you add comments at #else and #endif ?
include/mqtt/utf8encoded_strings.hpp
Outdated
| auto s = reinterpret_cast<const unsigned char*>(str.c_str()); | ||
| auto end = s + str.size(); | ||
|
|
||
| while (s < end) { |
There was a problem hiding this comment.
Is there any case s > end? If there is no case s != end is enough.
include/mqtt/utf8encoded_strings.hpp
Outdated
| #if !defined(MQTT_USE_STR_CK) | ||
| static_cast<void>(str); | ||
| #else | ||
| auto s = reinterpret_cast<const unsigned char*>(str.c_str()); |
There was a problem hiding this comment.
Could you replace const unsigned char* with unsigned char const*?
include/mqtt/utf8encoded_strings.hpp
Outdated
| if ((s[0] >= 0x01 && s[0] <= 0x1f) || s[0] == 0x7f) { | ||
| result = validation::well_formed_with_non_charactor; | ||
| } | ||
| s++; |
include/mqtt/utf8encoded_strings.hpp
Outdated
|
|
||
| while (s < end) { | ||
| if (s[0] < 0x80) { | ||
| // 0xxxxxxxxx |
There was a problem hiding this comment.
There are some comments that describe binary digit. But code is hex digit. You can use 0b10000000 if the code is clearer.
Added utf8 test code. Change the macro to MQTT_USE_STR_CHECK from MQTT_USE_STR_CK . Some bugfix and refactoring.
|
Thank you for updating, merged. |
Change checking utf8 string to 1.5.3 UTF-8 encoded strings specification from empty implementation.