Thanks for the review Johannes. I just pushed the following changes: * Include guard + license in constants.h * Replaced rogue tabs with spaces * Replace reference params with mixed return types * Use zend_parse_parameters_none for zero-arg functions * Use add_assoc_long_ex for constant string keys
I'll request an account. Adam On Fri, Sep 5, 2014 at 8:52 PM, Johannes Schlüter <[email protected]> wrote: > Hi, > > On Sat, 2014-08-23 at 03:59 -0400, [email protected] wrote: >> I've written a wrapper extension for termbox[1] that I think would >> make a nice addition to PECL. >> >> https://github.com/adsr/termbox-php >> >> Feedback appreciated! > > As said on IRC some comments from http://news.php.net/php.pecl.dev/12223 > apply, too. Additionally constants.h should have a license header and > include guard and you seem to mix space and tabs for indention. > > On an API design note I'd suggest that termbox_peek_event > termbox_poll_event don't return by reference but maybe an array like > [ "type" => TB_EVENT_*, "event" => [ ... ] ] on success and error code > or such on failure. Returning by reference is C-style and uncommon in > PHP. Also I think your way of returning by ref is wrong and will fail in > cases like $a = null; $b = $a; termbox_poll_event($a); as $b will be > changed, too, while it shouldn't. For receiving references you have to > tell the engine to separate before calling the function using ARGINFO. > In > http://schlueters.de/blog/archives/125-Do-not-use-PHP-references.html > http://schlueters.de/blog/archives/141-References-and-foreach.html > and https://www.youtube.com/watch?v=bxxIXPc9IR8 > I provide some reasons for not using references ... and frankly would > have to check myself on how to implement that properly. > > Aside from that it looks fine. > > If nobody else complains feel free to request PECL account for > publishing on pecl.php.net via http://pecl.php.net/account-request.php . > If you want a php.net account to host the primary git repo on > git.php.net (will be mirrored to github) and bug tracker make sure to > set the checkbox (unless you already have an account) and tell us so we > can enable that. > > johannes -- PECL development discussion Mailing List (http://pecl.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
