internals:review_comments

Differences

This shows you the differences between two versions of the page.

Link to this comparison view

internals:review_comments [2012/12/27 21:19]
johannes moved from internals:reciew_comments
internals:review_comments [2014/01/25 00:11] (current)
sixd Add title
Line 1: Line 1:
 +===== Common comments from reviewing PECL proposals =====
 +
 While reviewing PECL proposals there are some common issues which can be observed. This page collects some of those to provide extended information and saving reviewers from explaining those multiple times. This is different from other sections on this wiki a it focusses on review perspective. While reviewing PECL proposals there are some common issues which can be observed. This page collects some of those to provide extended information and saving reviewers from explaining those multiple times. This is different from other sections on this wiki a it focusses on review perspective.
  
Line 5: Line 7:
 ====== php_$extname.h should be minimal ====== ====== php_$extname.h should be minimal ======
  
-When reviewing extensions one can often see long //​php_$extname.h// ​headers ​which is bad. The //​php_$extname.h//​ header should only contain a limited set of declarations . It should not include library or system headers. ​Declarations ​for the extension'​s module ​entry and a module entry pointer.+When reviewing extensions one can often see long //​php_$extname.h// ​file, which is bad. The //​php_$extname.h//​ header should only contain a limited set of declarations. It should not include library or system headers. ​At most, it should contain declarations ​for the extension'​s module ​functions (like PHP_MINIT_FUNCTION), ​a module entry pointer ​and a version definition.
  
 If needed it may contain some public C-level APIs for other extensions. If there'​s a larger set of exported functionality these should get an extra header though. When providing such APIs the //​php_$extname.h//​ still shall not include other headers. If needed it may contain some public C-level APIs for other extensions. If there'​s a larger set of exported functionality these should get an extra header though. When providing such APIs the //​php_$extname.h//​ still shall not include other headers.
Line 40: Line 42:
 #endif #endif
 </​file>​ </​file>​
 +
 +====== Don't use C99 for portability reasons ======
 +
 +C99 is more than 10 years old. Still compiler vendors (most notably Microsoft Visual C) have only limited support of C99 features. Using C99 in an extension code can therefore limit the portability. ​ C99 features include ​
 +
 +  * inline functions
 +  * intermingled declarations and code: variable declaration is no longer restricted to file scope or the start of a compound statement (block), similar to C++
 +  * several new data types, including long long int, optional extended integer types, an explicit boolean data type, and a complex type to represent complex numbers
 +  * variable-length arrays
 +  * support for one-line comments beginning with %%.//%%., as in BCPL or C++
 +  * new library functions, such as snprintf
 +  * new header files, such as stdbool.h, complex.h, tgmath.h, and inttypes.h
 +  * type-generic math functions
 +  * improved support for IEEE floating point
 +  * designated initializers
 +  * compound literals
 +  * support for variadic macros (macros with a variable number of arguments)
 +  * restrict qualification allows more aggressive code optimization
 +  * universal character names, which allows user variables to contain other characters than the standard character set
 +
 +(Source: [[http://​en.wikipedia.org/​wiki/​C99#​Design]])
 +
 +A common mistake is the second item. Note that the macro TSRMLS_FETCH() includes a variable declaration. This macro should be the last item in the variable declaration. Code like this is invalid, according to this rule:
 +
 +  static long myext_do_something() {
 +    long a, b;
 +    a = myext_get_a();​
 +    TSRMLS_FETCH();​
 +    b = myext_get_b(TSRMLS_C);​
 +    long sum = a+b;
 +    return sum;
 +  }
 +  ​
 +This function might be written like this:
 +
 +  static long myext_do_something() {
 +    long a, b;
 +    long sum;
 +    TSRMLS_FETCH();​
 +    ​
 +    a = myext_get_a();​
 +    b = myext_get_b(TSRMLS_C);​
 +    sum = a+b;
 +    ​
 +    return sum;
 +  }
  
 ====== Don't use zend_error ====== ====== Don't use zend_error ======
Line 71: Line 119:
   ​   ​
 See also README.PARAMETER_PARSING_API in the PHP source tree for your version of PHP. See also README.PARAMETER_PARSING_API in the PHP source tree for your version of PHP.
 +
 +====== RINIT/​RSHUTDOWN should be as small as possible ======
 +
 +RINIT/​RSHUTDOWN are being called on every request handled by PHP, independently from whether the functionality is needed or not. Often it is a good alternative to delay initialisation of external libraries etc. which is commonly done in RINIT till the script access this for the first time or doing it globally in MINIT. RSHUTDOWN functionality can often be bound to resource/​object destructors.
 +
 +====== Empty RINIT/​RSHUTDOWN functions should be removed ======
 +
 +If RINIT/​RSHUTDOWN functions aren't needed they should be removed and the entries of the module entry structure should be set to NULL. Besides having less code = less chance of bugs this brings a tiny performance improvement as it saves 2 indirect function calls per request, which PHP would happen to execute on every request. A module structure might look like this:
 +
 +  zend_module_entry myext_module_entry = {
 +    STANDARD_MODULE_HEADER,​
 +    "​myext",​
 +    myext_functions,​
 +    PHP_MINIT(myext),​
 +    PHP_MSHUTDOWN(myext),​
 +    NULL,
 +    NULL,
 +    PHP_MINFO(myext),​
 +    PHP_MYEXT_VERSION,​
 +    STANDARD_MODULE_PROPERTIES
 +  };
 +  ​
 +If not needed one might also remove the function table, MINIT and MSHUTDOWN if those aren't needed, but as they are only accessed during the initial PHP startup, not for every request, this is less relevant.
 +
 +Also see the previous item.
 +
 +====== PECL site conformity ======
 +
 +A few conformity checks are done on the tarball upload, the PECL site will check for
 +
 +. presence of LICENSE or COPYING in the package.xml,​ add it to the root dir with the line like this
 +  ​
 +  <file name="​LICENSE"​ role="​doc"​ />
 +
 +. for version string compliance in package.xml and the extension source code. The following should be defined in one of the extension header files
 +
 +  #define PHP_EXTNAME_VERSION "​1.2.3"​
 +
 +This macros has to be used within your extname_module_entry to indicate the extension version. The version string in both package.xml and the defined macros have to match. The work of adding that macros could be already done when using ext_skel on PHP versions as of 5.4.21 or 5.5.5.
internals/review_comments.txt · Last modified: 2014/01/25 00:11 by sixd