internals:review_comments
no way to compare when less than two revisions
Differences
This shows you the differences between two versions of the page.
Previous revisionNext revision | |||
— | internals:review_comments [2013/01/09 11:17] – notes on C99 johannes | ||
---|---|---|---|
Line 1: | Line 1: | ||
+ | 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. | ||
+ | **Note to documentation team:** When adding this information to the manual please keep a reference on this page! | ||
+ | |||
+ | ====== php_$extname.h should be minimal ====== | ||
+ | |||
+ | When reviewing extensions one can often see a long // | ||
+ | |||
+ | If needed it may contain some public C-level APIs for other extensions. If there' | ||
+ | |||
+ | The reason for this is that for a static build of the extension as part of the main PHP build the PHP configure script will create // | ||
+ | |||
+ | A good starting point for a // | ||
+ | |||
+ | < | ||
+ | /* | ||
+ | +----------------------------------------------------------------------+ | ||
+ | | PHP Version 5 | | ||
+ | +----------------------------------------------------------------------+ | ||
+ | | Copyright (c) 1997-2012 The PHP Group | | ||
+ | +----------------------------------------------------------------------+ | ||
+ | | This source file is subject to version 3.01 of the PHP license, | ||
+ | | that is bundled with this package in the file LICENSE, and is | | ||
+ | | available through the world-wide-web at the following url: | | ||
+ | | http:// | ||
+ | | If you did not receive a copy of the PHP license and are unable to | | ||
+ | | obtain it through the world-wide-web, | ||
+ | | license@php.net so we can mail you a copy immediately. | ||
+ | +----------------------------------------------------------------------+ | ||
+ | | Author: Ted User < | ||
+ | +----------------------------------------------------------------------+ | ||
+ | */ | ||
+ | |||
+ | #ifndef PHP_EXTNAME_H | ||
+ | #define PHP_EXTNAME_H | ||
+ | |||
+ | extern zend_module_entry extname_module_entry; | ||
+ | #define phpext_extname_ptr & | ||
+ | |||
+ | #endif | ||
+ | </ | ||
+ | |||
+ | ====== 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. | ||
+ | |||
+ | * 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:// | ||
+ | |||
+ | 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 ====== | ||
+ | |||
+ | // | ||
+ | |||
+ | ====== If zend_parse_parameters() fails do a simple return ====== | ||
+ | |||
+ | The // | ||
+ | |||
+ | > **Note:** If the parameters given to a function are not what it expects, such as passing an array where a string is expected, the return value of the function is undefined. In this case it will likely return **NULL** but this is just a convention, and cannot be relied upon. | ||
+ | (Source: [[http:// | ||
+ | |||
+ | Even though this is a convention this rule should be followed unless there are good reasons. | ||
+ | |||
+ | Those are wrong: | ||
+ | |||
+ | if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, " | ||
+ | RETURN_FALSE; | ||
+ | } | ||
+ | if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, " | ||
+ | php_error_docref(NULL, | ||
+ | return; | ||
+ | } | ||
+ | |||
+ | This form is suggested: | ||
+ | |||
+ | if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, " | ||
+ | return; | ||
+ | } | ||
+ | | ||
+ | See also README.PARAMETER_PARSING_API in the PHP source tree for your version of PHP. | ||
+ | |||
+ | ====== RINIT/ | ||
+ | |||
+ | RINIT/ | ||
+ | |||
+ | ====== Empty RINIT/ | ||
+ | |||
+ | If RINIT/ | ||
+ | |||
+ | zend_module_entry myext_module_entry = { | ||
+ | STANDARD_MODULE_HEADER, | ||
+ | " | ||
+ | myext_functions, | ||
+ | PHP_MINIT(myext), | ||
+ | PHP_MSHUTDOWN(myext), | ||
+ | NULL, | ||
+ | NULL, | ||
+ | PHP_MINFO(myext), | ||
+ | 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. |
internals/review_comments.txt · Last modified: 2017/09/22 13:28 by 127.0.0.1