Hi Anatol,

Thanks a lot. I've made the suggested changes.

- I removed the SODIUM references.
- For base58_encode I now use zend_string directly. For base58_decode I
copy the char* into a zend_string as it has an offset. Also, I have sure I
free everything on an error.
- I considered returning NULL rather than FALSE as suggested in the review
comments page. But it's probably preferable if it works the same as the
base64 functions.

I've added automated builds and testing on Travis.

I'm also trying to build on Windows using AppVoyer. However, I lack any
skill and knowledge of working with Windows. I'll try to get that sorted.

Regards,
Arnold


On Sat, May 19, 2018 at 8:27 PM Anatol Belski <a...@php.net> wrote:

> 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