Add base32 decode base32 encode #1
Conversation
|
|
||
| smart_str_appends(&unique_chars, c); | ||
| } | ||
|
|
There was a problem hiding this comment.
For reference the part that validates the padding and the alphabet should be extract in an internal function as this part is identical for both base32_encode and base32_decode functions,
So I believe one or two inline static functions should be used if I understood correctly 🤔
There was a problem hiding this comment.
Yes, you should use a static function for that, but marking it as inline is probably not needed, as compilers usually decide this on their own, for only small functions.
|
|
||
| zend_string *upper_alpha = zend_string_toupper(alphabet); | ||
| zend_string *upper_padding = zend_string_toupper(padding); | ||
| zend_string *reserved_chars = zend_string_concat2(reserved, 4, ZSTR_VAL(upper_padding), 1); |
There was a problem hiding this comment.
These three calls, allocated new memory. That means that this function should also be responsible for freeing them, before returning with zend_string_release(). You return in lines 63, 71, 80, and 106. It is not unheard of to use goto failure, with a failure: block and the end of the function for this.
You could also only call zend_string_* just before they are actually needed. The new upper_padding and reserved_chars aren't used in the section on lines 61-63, so they could come below.
You can find this kind of memory leak, by compiling PHP in debug mode with the --enable-debug flag. When running your tests, you should then see warnings.
Alternatively, you can run your tests with php run-tests.php TESTS="-m ext/standard/tests/url" — the -m will use valgrind (which you need to install) to run memory analyses.
| Z_PARAM_STR(padding) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| if (ZSTR_LEN(padding) != 1) { |
There was a problem hiding this comment.
I would probably move all the things below into a php_base32_encode() function, just like is done for base64 in the PHP_FUNCTION(base64_encode)/PHP_FUNCTION(base64_decode) functions in ext/standard/base64.c. You will also have to add an equivalent PHPAPI extern zend_string *php_base64_encode(...) declaration in base32.h (again, see how base64.h has done that).
By splitting it out, you make this function available to other PHP extensions as well.
| zend_string *upper_padding = zend_string_toupper(padding); | ||
| zend_string *reserved_chars = zend_string_concat2(reserved, 4, ZSTR_VAL(upper_padding), 1); | ||
|
|
||
| if (strcspn(ZSTR_VAL(upper_alpha), reserved_chars) != 32) { |
There was a problem hiding this comment.
There is a PHP variant of strcspn, called php_strcspn (defined in ext/standard/php_string.h). I think because not all platforms might have it. The API is slightly different, but it is NULL safe:
PHPAPI size_t php_strspn(const char *s1, const char *s2, const char *s1_end, const char *s2_end);
| smart_str unique_chars = {0}; | ||
| for (int i = 0; i < 32; i++) { | ||
| char c = upper_alpha[i]; | ||
| if (strstr(unique_chars, c)) { |
There was a problem hiding this comment.
There is a NULL-safe PHP variant of strstr too: zend_memnstr(const char *haystack, const char *needle, size_t needle_len, const char *end);
|
|
||
| smart_str_appends(&unique_chars, c); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yes, you should use a static function for that, but marking it as inline is probably not needed, as compilers usually decide this on their own, for only small functions.
| RETURN_EMPTY_STRING(); | ||
| } | ||
|
|
||
| char chars[] = ZSTR_VAL(decoded); |
There was a problem hiding this comment.
That's probably usually defined as: const char *chars = ZSTR_VAL(decoded);.
The [] isn't really wrong, but not common.
The const indicates that the function is not going to modify the contents, which is a good hint to the compiler (and reader of code).
| bitLen -= 5; | ||
| } | ||
|
|
||
| zend_string *ret_val = smart_str_extract(encoded); |
There was a problem hiding this comment.
You haven't freed the encoded smart_str, like you did above for with smart_str_free(&unique_chars);.
In this case, that is right, as RETURN_STR(ret_val) doesn't create a duplicate.
Usually, it's folded into one line though:
RETURN_STR(smart_str_extract(&buf));
No description provided.