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

Reply via email to