Hi, On Tue, 2018-05-15 at 21:26 +0200, Arnold Daniels wrote: > Hi, > > I'd like to request the submission of a package to PECL. > > We've created a PHP extension for base58 encoding and decoding using > the > Bitcoin alphabet. It's a simple binding to the bitcoin/libbase58 c > library. > > The source code can be found here: > > I saw your request on twitter and came to look over the ext right now.
In the file php_base58.h, the function declarations like MINFO and others are not necessarily need to be there. Please check here for more info https://wiki.php.net/internals/review_comments . Those prototypes don't need to be exported to the outta world. Also seems you forgot to replace things like PHP_SODIUM_API with the actual ext name, same in config.w32. The linked document also contains several other common things, that you could check. For base58.c actually some minor things as well - With RETURN_STRINGL there might be a memory leak, as it would actually allocate a new zend_string, so the old emalloc'd pointer might be leaked. I had no time to test, please run under valgrind. It might be anyway more convenient to allocate a zend_string at once and then use RETVAL_STR or alike. - TSRMLS_CC is not needed, as you target 7.0+ - C style comments should be used The code in the ext is accessable, so actually looks good. > *Why not userspace functions?* > There are some implementations in userspace ( > https://github.com/stephen-hill/base58php). These rely on bcmath or > gmp and > are much to slow to handle more than a few bytes. This base58 > extension > performs as well as comparable functions like `base64_encode`. > Yes, where it can be faster using native C implementations for the same functionality, why not. Later for the Windows build libbase58 will need to be put on the build bot. Regards Anatol