Re: termbox-php

Hi, On Sat, 2014-08-23 at 03:59 -0400, adam@atoi.cc 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

Attachment: [application/pgp-signature] This is a digitally signed message part signature.asc